Introduce the possibility to customize the help printer function #1342
Conversation
|
could you resolve the CLA issue please? |
|
@dominichamon I was sure to have tested it in my branch, sorry I will fix right now. |
no problem at all.. as well as the test failures it looks like you'll need to click the CLA and sign it before we can take your patch. |
vincenzopalazzo
force-pushed
the
vincenzopalazzo/help_function
branch
2 times, most recently
from
9b2b326
to
5c8988a
Compare
Right, I did it, thanks |
vincenzopalazzo
force-pushed
the
vincenzopalazzo/help_function
branch
2 times, most recently
from
f036c54
to
d63fea0
Compare
include/benchmark/benchmark.h
Outdated
| @@ -1203,6 +1204,24 @@ class LambdaBenchmark : public Benchmark { | |||
| }; | |||
| #endif | |||
|
|
|||
| inline void helper_printer() { | |||
There was a problem hiding this comment.
does this need to be inline? (nit, it should be HelperPrinter)
There was a problem hiding this comment.
Thanks to catch my naming convention error, sorry my last C experience changes my naming convention in my mind :/ Sorry.
In addition, I use the inline function only because I have some redefinition function at compiler time.
The solution, in this case, can be:
- define the prototype of the function in the .h file and add the definition in the .cc file;
- use the nullprt in the function and define an HelperPrinter in the .cc file, so we can hide all the logic.
In both cases, we have some not clear states of the HelperPrintf function that is used to store the procedure to print the help information.
Maybe it is my missing some knowledge of c++ in this case?
There was a problem hiding this comment.
defining the prototype (declaring) in the header and having the implementation (definition) in the .cc file seems reasonable to me. there's no reason i can see to have the implementation in the header.
There was a problem hiding this comment.
defining the prototype (declaring) in the header and having the implementation (definition) in the .cc file seems reasonable to me. there's no reason i can see to have the implementation in the header.
With the following solution, I had some problems with the dynamic linker (https://github.com/vincenzopalazzo/benchmark/runs/5204928772?check_suite_focus=true) and I implemented your first suggestion to use the NULL pointer.
Open to other suggestions to improve the actual solution.
There was a problem hiding this comment.
it looks like you maybe had the definition in the wrong place... maybe it wasn't in the benchmark namespace but in the benchmark::internal namespace.
either way, i think your current version is clean. waiting for the bots to agree.
There was a problem hiding this comment.
ops, maybe I made this stupid error, but was a good moment to try the following solution, that introduces less code, that it is always good IMHO
vincenzopalazzo
force-pushed
the
vincenzopalazzo/help_function
branch
2 times, most recently
from
50e78b2
to
f001ed4
Compare
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
…t method Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
vincenzopalazzo
force-pushed
the
vincenzopalazzo/help_function
branch
from
9bef90f
to
6aed34d
Compare
|
thanks |
|
Thank you for your time. I will be around, so if there are other issues where you would like to see into it, just assign them to me. |
This is the first attempt to introduce an help_printer function.
But there are a few things that I don't like, such as:
stdioin thebenchamark.hhprintfinbenchmark.ccBut I think it is a good starting point to start to discuss about it
Fixes #1329