You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
When traces are disabled, trace_* calls shouldn't expand to nothing #1999
I claim that when trace_* expands to nothing then it will be ok, but when trace_* expand to any code with more than one line of code without { } inside then it won't. It will be need to spend much more time to debug what happen in such a situation in comparison with usage redundant parenthesis. I know that such a macro function is not a good solution but it is open source project and when using extra {} I know that I am safe for such a situation. Such a situation is not a big abstraction, also in tracing functions, for example:
will trace independently ret value but my solution will still work correctly. There also won't be any compilation error and a lot of tests should pass. In conclusion, I rather prefer defensive programming with extra {} from debugging.
@ktrzcinx This one is actually a non-issue as all multiline macros either are uppercase (obvious macros) or are wrapped around a do { ... } while(0) block. When I extended the trace function for my own platform (to also call some __dsp_printf() function which you can see in paulstelian97:topic/imx_wip) I wrap it around a do {...} while(0). That's what all macros which don't have uppercase macro-like names should do.
This is a multiline macro from within trace. To make trace_* macros act as functions this has been made into a do {...} while(0) macro. So in your case no need for the extra safety as the convention is that any bugs coming from weird flow control from macros with lowercase names are bugs in the macros themselves and need to be adjusted. Hence my issue.
Of course it is as you said. The only difference is that extra { } allow to defense from errors in macros written out of convention and lack of then lead to malfunctioning code.
Guess you can add it in, my issue still stands though -- the macros themselves are written like functions and should act like functions, thus should be safe even if you don't take care like that.
@ktrzcinx I think the better defensive technique is to have macros wrapped around do {} while(0) rather then expecting anyone using curly braces with if constructs.
I found some bigger trouble with previous code and at the opportunity I removed this { } as you wish but I still think that the best options is using do { } while(0) in multiline macro functions and { } in each if statement were content is not just C/C++ keyword or rather in each if/while statement to be more consistent.
Well unless someone else picks up on this before me I'll get to fixing this one myself once the asserts change is merged.
Not having {} on if statements unless required by one of the branches is something coming from the Linux kernel coding style, which is also used within SOF. So:
if (cond)
action1;
elseaction2;
But:
if (cond) {
multiline1;
multiline2;
} else {
single_action;
}
Activity
ktrzcinx commentedon Oct 24, 2019
I claim that when
trace_*expands to nothing then it will be ok, but whentrace_*expand to any code with more than one line of code without{ }inside then it won't. It will be need to spend much more time to debug what happen in such a situation in comparison with usage redundant parenthesis. I know that such a macro function is not a good solution but it is open source project and when using extra{}I know that I am safe for such a situation. Such a situation is not a big abstraction, also in tracing functions, for example:or
Now
will trace independently ret value but my solution will still work correctly. There also won't be any compilation error and a lot of tests should pass. In conclusion, I rather prefer defensive programming with extra
{}from debugging.paulstelian97 commentedon Oct 24, 2019
@ktrzcinx This one is actually a non-issue as all multiline macros either are uppercase (obvious macros) or are wrapped around a
do { ... } while(0)block. When I extended the trace function for my own platform (to also call some __dsp_printf() function which you can see in paulstelian97:topic/imx_wip) I wrap it around ado {...} while(0). That's what all macros which don't have uppercase macro-like names should do.paulstelian97 commentedon Oct 24, 2019
A particular example of such is:
This is a multiline macro from within trace. To make trace_* macros act as functions this has been made into a
do {...} while(0)macro. So in your case no need for the extra safety as the convention is that any bugs coming from weird flow control from macros with lowercase names are bugs in the macros themselves and need to be adjusted. Hence my issue.ktrzcinx commentedon Oct 24, 2019
Of course it is as you said. The only difference is that extra
{ }allow to defense from errors in macros written out of convention and lack of then lead to malfunctioning code.paulstelian97 commentedon Oct 24, 2019
Guess you can add it in, my issue still stands though -- the macros themselves are written like functions and should act like functions, thus should be safe even if you don't take care like that.
dbaluta commentedon Oct 24, 2019
@ktrzcinx I think the better defensive technique is to have macros wrapped around
do {} while(0)rather then expecting anyone using curly braces with if constructs.ktrzcinx commentedon Oct 25, 2019
I found some bigger trouble with previous code and at the opportunity I removed this
{ }as you wish but I still think that the best options is usingdo { } while(0)in multiline macro functions and{ }in each if statement were content is not just C/C++ keyword or rather in each if/while statement to be more consistent.paulstelian97 commentedon Oct 25, 2019
Well unless someone else picks up on this before me I'll get to fixing this one myself once the asserts change is merged.
Not having
{}onifstatements unless required by one of the branches is something coming from the Linux kernel coding style, which is also used within SOF. So:But: