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

Fixups: build with debug enabled #67

Merged
merged 4 commits into from Jul 16, 2013
Merged

Fixups: build with debug enabled #67

merged 4 commits into from Jul 16, 2013

Conversation

mehlis
Copy link
Contributor

@mehlis mehlis commented Jul 15, 2013

No description provided.

LudwigKnuepfer added a commit that referenced this pull request Jul 16, 2013
Fixups: build with debug enabled
@LudwigKnuepfer LudwigKnuepfer merged commit f1474e4 into RIOT-OS:master Jul 16, 2013
@LudwigKnuepfer
Copy link
Member

Well, of course it saves a few keystrokes, but // hurts the coding conventions - enabling /* #define ENABLE_DEBUG */ saves even less keystrokes ;-)
Either way - in cleaned up code vs. ease of typing I tend to prefer tidier code. To me this case appears to be a matter of personal preference and I don't feel strongly about it.

Since the request has already been merged ... do you want the commits to be reverted?
Or, should we add something like commented out debugging code and/or ENABLE_DEBUG does not count as clutter to our guidelines and just leave the defines/comments the next time they are reintroduced?

@kaspar030
Copy link
Contributor

+1 for tidier code, no need to revert. Could we globally define DEBUG(...) as empty and so get rid of the debug.h include as well?

or better, globally define DEBUG as something like do { if (ENABLE_DEBUG) printf(fmt, __VA_ARGS__); } while (0) and have ENABLE_DEBUG default to 0.
That way the compiler always checks the validity of the debug code.

@OlegHahm
Copy link
Member

Hmm, basically a good idea, but that means that we rely on every compiler for every obscure MCU to be smart enough to remove the if-condition. I guess that's ok?

@kaspar030
Copy link
Contributor

It would also compile in the check when using -O0, which might be a problem even on the newest compilers.

@OlegHahm
Copy link
Member

But who would compile the code with -O0 and care about performance?

@OlegHahm
Copy link
Member

But what about this solution: 599e266 ?

chrysn pushed a commit to chrysn-pull-requests/RIOT that referenced this pull request Sep 16, 2022
RIOT: update submodule to 2019.10 release
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.

None yet

4 participants