Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds empty dbg calls #67

Closed
wants to merge 4 commits into from

Conversation

DerekCresswell
Copy link
Contributor

@DerekCresswell DerekCresswell commented Dec 15, 2019

This allows users to call dbg() with no arguments to print out dummy text "dbg call reached".
Works with or without DBG_MACRO_DISABLE being defined.
Closes #64

@DerekCresswell DerekCresswell changed the title Adds empty dbg calls, closes #64 Adds empty dbg calls Dec 15, 2019
@sharkdp
Copy link
Owner

sharkdp commented Dec 16, 2019

Thank you very much for your contribution!

I didn't know that this would require us to add a lot of preprocessor macro code 🙁.

If we decide to add all of these tricks, (just) to support empty dbg() statements, do you think that there is a way to also make this compatible with #2?

dbg.h Outdated Show resolved Hide resolved
@DerekCresswell
Copy link
Contributor Author

DerekCresswell commented Dec 17, 2019

@sharkdp I will change the print function to remove that duplication.

As for the macro, there really is not a way to do this with pre-processor statements other than to count the VA. Though adding other number of variables should be trivial. I.E.

#define dbg_x(x, A, B, Func, ...) Func
// ---
#define dbg_2(A, B) // ---
// ---
#define dbg(...) dbg_x(, ##__VA_ARGS__, dbg_VA(__VA_ARGS__), dbg_2(__VA_ARGS__),dbg_0())

This should not greatly affect the ability to implement #2 but I can attempt to look into it more. The only possible "snag" I see is needing defining a dbg_1(A).
This does what we need for now and shouldn't change the future much.

@DerekCresswell DerekCresswell force-pushed the add-empty-calls branch 2 times, most recently from d2c7f62 to 4a77805 Compare December 17, 2019 06:43
@DerekCresswell
Copy link
Contributor Author

DerekCresswell commented Dec 17, 2019

Alright @sharkdp the duplicate code is gone.
I've looked at the printing two arguments and am quite confident I could get it to work. Only thing is because of the lack of recursion for macros it will be like what I have here just expanded. It might look fairly ugly and become less readable.
I believe we should mostly ditch #2 as it will create a messy code base, though I will see if I can come up with something nice. I also think the functionality of empty calls is better than that of 2 arguments.

Let me know if you have more questions or concerns.

@sharkdp
Copy link
Owner

sharkdp commented Dec 19, 2019

I've looked at the printing two arguments and am quite confident I could get it to work. Only thing is because of the lack of recursion for macros it will be like what I have here just expanded. It might look fairly ugly and become less readable.
I believe we should mostly ditch #2 as it will create a messy code base, though I will see if I can come up with something nice. I also think the functionality of empty calls is better than that of 2 arguments.

To be honest, for me, the multiple-arguments-dbg-call would be more interesting than than the zero-argument-dbg-call.

We did have an implementation for multiple arguments in #8 (which I needed to roll back because it broke some tests). It's not pretty, but I as you also said, there is no other way apparently. Ideally, this preprocessor code will not have to be touched in the future, as soon as we have support for 0 or more arguments.

The limitation to a certain maximum number of arguments wouldn't be any problem, in my point of view.

Thank you very much for looking into this!

@DerekCresswell
Copy link
Contributor Author

@sharkdp Would you like me to try and put multiple arguments onto this as #64 and #2 do go hand in hand?
Say 0, 1, and 2 arg macros? How many would we want? How would they printout, just a print() for each one?

Or is there another issue / feature that'd be a better use of time here? I'm asking because I really enjoy this repo and would love to start actually working on repos other than mine :)

@sharkdp
Copy link
Owner

sharkdp commented Dec 19, 2019

@sharkdp Would you like me to try and put multiple arguments onto this as #64 and #2 do go hand in hand?

That would be fantastic.

Say 0, 1, and 2 arg macros? How many would we want?

I would hope that if 0, 1 and 2 are possible, we could also add a few more lines to support 5 or 10.

How would they printout, just a print() for each one?

One line for each argument. Just as if the user had called dbg(arg1); dbg(arg2); dbg(arg3);. In an expression, the last argument would be returned. So basically, dbg(arg1, arg2, arg3) would be equivalent to dgb(arg1), dbg(arg2), dbg(arg3).

@DerekCresswell DerekCresswell changed the title Adds empty dbg calls WIP: Adds empty dbg calls Dec 20, 2019
@DerekCresswell DerekCresswell changed the title WIP: Adds empty dbg calls Adds empty dbg calls Dec 28, 2019
@sharkdp
Copy link
Owner

sharkdp commented Dec 28, 2019

Here is the error:

C:\projects\dbg-macro\tests\example.cpp(28): error C2059: syntax error: ')' [C:\projects\dbg-macro\build\dbg_macro-example.vcxproj]
C:\projects\dbg-macro\tests\example.cpp(28): error C3553: decltype expects an expression not a type [C:\projects\dbg-macro\build\dbg_macro-example.vcxproj]
C:\projects\dbg-macro\tests\example.cpp(28): error C2947: expecting '>' to terminate template-argument-list, found '>' [C:\projects\dbg-macro\build\dbg_macro-example.vcxproj]

Looks like this doesn't work on MSVC.

dbg can now be called like dbg() with no arguments. Works by using the
VA_ARGS to determine how many arguements have been passed in via dbg_x.
Also works just fine if DBG_MACRO_DISABLE is defined.
Seeing as the dbg(...) was the same whether disabled or not I've moved
it beside the dbg_x() definition outside of the disable if.
Simply put some empty calls onto tests/tests.cpp an tests/example.cpp.
Also for consistency I've updated the README.md to have the empty call
in it.

This should be all that is needed to implement empty calls. :)
@sharkdp
Copy link
Owner

sharkdp commented Dec 28, 2019

(I fixed some merge conflicts)

With clang, I get a lot of warnings:

/home/shark/Dropbox/Informatik/c++/dbg-macro/./dbg.h:570:24: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
#define dbg(...) dbg_x(, ##__VA_ARGS__, dbg_VA(__VA_ARGS__), dbg_0())

@sharkdp
Copy link
Owner

sharkdp commented Dec 28, 2019

On gcc, I also get some warnings:

/home/shark/Dropbox/Informatik/c++/dbg-macro/tests/tests.cpp: In function ‘int main()’:
/home/shark/Dropbox/Informatik/c++/dbg-macro/tests/tests.cpp:137:27: warning: statement has no effect [-Wunused-value]
  137 |   dbg(std::vector<int>{0, 1, 0, 1});
      |                           ^
/home/shark/Dropbox/Informatik/c++/dbg-macro/./dbg.h:566:32: note: in definition of macro ‘dbg_x’
  566 | #define dbg_x(x, A, Func, ...) Func
      |                                ^~~~
/home/shark/Dropbox/Informatik/c++/dbg-macro/tests/tests.cpp:137:3: note: in expansion of macro ‘dbg’
  137 |   dbg(std::vector<int>{0, 1, 0, 1});
      |   ^~~

I'm beginning to think that we should simply omit this feature 🙁

@DerekCresswell
Copy link
Contributor Author

I'm beginning to think that we should simply omit this feature

Let me keep at her a bit longer, don't lose hope. A lot of warnings are because compilers do not like (, ##__VA_ARGS__) even though they support it. I'll find a way. Got my windows laptop now too.

@sharkdp
Copy link
Owner

sharkdp commented Mar 9, 2020

Any update on this? Otherwise, I'd like to close it.

@DerekCresswell
Copy link
Contributor Author

Sadly no, been neglecting this : (. I'd say keep the issue open though.

@sharkdp
Copy link
Owner

sharkdp commented Mar 9, 2020

No worries, thank you for the update. We'll keep the issue open, yes. The PR here is not lost either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for empty "dbg()" calls.
2 participants