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

Support for multiple comma-separated arguments? #2

Closed
sharkdp opened this issue Jun 19, 2019 · 17 comments · Fixed by #8 or #94
Closed

Support for multiple comma-separated arguments? #2

sharkdp opened this issue Jun 19, 2019 · 17 comments · Fixed by #8 or #94
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@sharkdp
Copy link
Owner

sharkdp commented Jun 19, 2019

dbg(42, "hello world");
hmenke added a commit to hmenke/dbg-macro that referenced this issue Jun 20, 2019
hmenke added a commit to hmenke/dbg-macro that referenced this issue Jun 20, 2019
sharkdp added a commit that referenced this issue Jun 20, 2019
@ShikChen
Copy link
Contributor

Could we reopen this issue since the commit is reverted?

Quote from #8 (comment)

I actually just reverted the merge - I should have tested this first, sorry. Allowing for multiple arguments like I described in #2 is not compatible with using dbg(…) in expressions. I would really like to keep that functionality.

I should have thought about this before writing #2.

If we allow for dbg(x, y, …), we would have to make a rather arbitrary choice of just returning x.\

I think multiple arguments is a more common use case than returning from expressions. I have a similar debugging macro locally and I use it with multiple args from day to day. If we would like to support them at the same time, there are some possible routes:

  1. Only returns x from dbg(x), and returns void for multiple args case.
  2. Similar to Rust, returns a tuple when there are multiple args.

@sharkdp sharkdp reopened this Jul 24, 2019
@sharkdp
Copy link
Owner Author

sharkdp commented Jul 24, 2019

I think multiple arguments is a more common use case than returning from expressions.

Good point. I think the dbg(…)-inside-expressions usage is actually surprisingly useful, once you get used to it, but I would tend to agree with your statement.

  • Only returns x from dbg(x), and returns void for multiple args case.
  • Similar to Rust, returns a tuple when there are multiple args.

I think I would go with the first option and return void (or even "disallow" the usage with multiple arguments within an expression). I don't think it's very likely that someone will work with the tuple that would be returned in the second case.

@travisdowns
Copy link

I second the case that multiple arguments is very useful. I had already used this widely before the change was reverted, so for now I'm stuck on the old version of dbg() until/if this is fixed.

A third option would be to return the last value, similar to how the comma operator works.

@sharkdp
Copy link
Owner Author

sharkdp commented Aug 1, 2019

I actually like that even better, just return the last argument.

@sharkdp
Copy link
Owner Author

sharkdp commented Dec 27, 2019

Unfortunately, this request has to be closed.

There is no clean way to implement multiple argument support without losing another feature: unprotected commas within the dbg(…) call. This is due to a limitation of the preprocessor which does not properly parse commas within curly {…} braces or template argument lists <…>. So things like

dbg(MyClass{1,2,3});

or

dbg(my_template_function<int, 2>());

wouldn't work anymore.

Even if those things would still work with additional parenthesis (dbg((…))), I think that it would be surprising for a lot of users.

For a detailed discussion, see #64.

@sharkdp sharkdp closed this as completed Dec 27, 2019
@ShikChen
Copy link
Contributor

Yes there is no perfect solution here, but I would lean to support multiple arguments instead of unprotected commas:

  • Multiple arguments is a common needed scenario and no easy workaround.
  • Unprotected commas can be workaround by adding parenthesis, and is widely used in popular libs, such as googletest, and Catch2.

@sharkdp
Copy link
Owner Author

sharkdp commented Dec 29, 2019

Fair points. Opening this once again 😄

Multiple arguments is a common needed scenario and no easy workaround.

well, the workaround is to dbg(x); dbg(y); dbg(z); instead of dbg(x, y, z);

@sharkdp sharkdp reopened this Dec 29, 2019
@sharkdp
Copy link
Owner Author

sharkdp commented Dec 29, 2019

Unprotected commas can be workaround by adding parenthesis, and is widely used in popular libs, such as googletest, and Catch2.

Catch2 is also a counter-example. For all of its one-argument macros (CHECK(…), REQUIRE(…), ..), you can use unprotected commas.

@ShikChen
Copy link
Contributor

Fair points. Opening this once again 😄

Multiple arguments is a common needed scenario and no easy workaround.

well, the workaround is to dbg(x); dbg(y); dbg(z); instead of dbg(x, y, z);

Ah sure.

Unprotected commas can be workaround by adding parenthesis, and is widely used in popular libs, such as googletest, and Catch2.

Catch2 is also a counter-example. For all of its one-argument macros (CHECK(…), REQUIRE(…), ..), you can use unprotected commas.

That's why I link to the CAPTURE() macro directly :P

Catch2 even parses / splits the expressions in the CAPTURE() macro by itself, so it can handle some of the unprotected commas. Note that because C++ is a context sensitive, thus it's quite hard (if not impossible) to disambiguate code like X<0>(1) correctly in this way.

Another hybrid approach is making output as x, y = 1, 2 instead of x = 1, y = 2. The names are just taken from dbg(...) directly, and the values are forwarded to a variadic template function, which should parse the arguments correctly.

@sharkdp
Copy link
Owner Author

sharkdp commented Dec 29, 2019

Another hybrid approach is making output as x, y = 1, 2 instead of x = 1, y = 2. The names are just taken from dbg(...) directly, and the values are forwarded to a variadic template function, which should parse the arguments correctly.

Hm. I don't quite understand. How would this work?

@ShikChen
Copy link
Contributor

Here is a minimal proof-of-concept:

#include <iostream>
#include <utility>

template <class T, class... U>
void dbg_impl(T &&head, U &&... tail) {
  std::cerr << head;
  if constexpr (sizeof...(U) > 0) {
    std::cerr << ", ";
    dbg_impl(std::forward<U>(tail)...);
  } else {
    std::cerr << "\n";
  }
}

#define dbg(...)                        \
  do {                                  \
    std::cerr << #__VA_ARGS__ << " = "; \
    dbg_impl(__VA_ARGS__);              \
  } while (false)

int main() {
  int x = 1;
  int y = 2;
  dbg(x, y, std::pair<int, int>(x, y).first);
  return 0;
}

It will output:

x, y, std::pair<int, int>(x, y).first = 1, 2, 1

@sharkdp
Copy link
Owner Author

sharkdp commented Jan 1, 2020

@ShikChen If we can make this work with the type-output as well (and pass all the tests that are currently in place), I think that would be a great compromise.

@travisdowns
Copy link

travisdowns commented Jan 4, 2020

  • Unprotected commas can be workaround by adding parenthesis, and is widely used in popular libs,

Came here to say this, but @ShikChen beat me to it.

@sharkdp
Copy link
Owner Author

sharkdp commented Jan 4, 2020

Okay, so you are voting to drop the "commas inside dbg(…)" feature in favor of multiple arguments? I think I'm now also inclined to do so.

In this case, I would prefer the solution where dbg(x, y, z) has the same output as dbg(x), dbx(y), dbg(z).

@sharkdp
Copy link
Owner Author

sharkdp commented Jan 4, 2020

Note that there is an implementation of multiple arguments here: #8. It was reverted because it did not return the value, but I guess that could be fixed easily. Not sure if there are other implementations that would need less preprocessor code.

@sharkdp sharkdp mentioned this issue Mar 3, 2020
@sharkdp sharkdp added the enhancement New feature or request label Mar 9, 2020
@sharkdp sharkdp added the help wanted Extra attention is needed label Mar 9, 2020
@ShikChen
Copy link
Contributor

I have a simple proof of concept, and I think we need to decide what's the expected behavior when multiple arguments are used with the special helpers that don't print the type and expressions, such as dbg(dbg::time(), 1 + 2).

Currently there are 3 special cases we dbg() won't print the type and original expression:

  1. C string literal (const char (&value)[N])
  2. dbg::time
  3. dbg::type

Here is an example:

#include <dbg.h>

#define NAME "bar"
typedef uint64_t u64;

int main() {
  dbg("foo");
  dbg(NAME);
  dbg(dbg::time());
  dbg(dbg::type<u64>());
  // dbg(dbg::time(), 1 + 2, "meow");  // won't compile in the current version
  return 0;
}

Current output is:

[shik.cc:7 (main)] foo
[shik.cc:8 (main)] bar
[shik.cc:9 (main)] current time = 18:25:00.066772
[shik.cc:10 (main)] unsigned long long [sizeof: 8 byte, trivial: yes, standard layout: yes]

I personally prefer to output:

[shik.cc:7 (main)] foo
[shik.cc:8 (main)] NAME = bar (const char*)
[shik.cc:9 (main)] dbg::time() = 18:25:00.066772 (dbg::time)
[shik.cc:10 (main)] dbg::type<u64>() = unsigned long long [sizeof: 8 byte, trivial: yes, standard layout: yes] (dbg::type)
[shik.cc:11 (main)] dbg::time(), 1 + 2, "meow" = 18:25:00.067083, 3, meow (dbg::time, int, const char*)

Does it look ok to you?

(I also prefer to shorten the type info as [8 bytes, trivial, standard layout] and reduce the timestamp precision to millisecond, but that's orthogonal to the multiple arguments issue)

@sharkdp
Copy link
Owner Author

sharkdp commented Mar 28, 2020

I have a simple proof of concept

That sounds great!

and I think we need to decide what's the expected behavior when multiple arguments are used with the special helpers that don't print the type and expressions

Why? My preferred way how multiple arguments would work would be that the output of dbg(x, y, z); is the same as dbg(x); dbg(y); dbg(z);. I find the x, y, z = x_value, y_value, z_value a bit confusing and non-optimal.

[shik.cc:8 (main)] NAME = bar (const char*)

I'm pretty sure that wouldn't work because NAME will be replaced by the preprocessor.

(I also prefer to shorten the type info as [8 bytes, trivial, standard layout] and reduce the timestamp precision to millisecond, but that's orthogonal to the multiple arguments issue)

Yeah, let's discuss this in separate tickets.

ShikChen added a commit to ShikChen/dbg-macro that referenced this issue Apr 26, 2020
sharkdp pushed a commit that referenced this issue May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
3 participants