[proguard] Change proguard warnings to TRACE statements #516
Conversation
|
Hi @danzimm! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but we do not have a signature on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
danzimm
force-pushed
the
pg
branch
2 times, most recently
from
c86bb66
to
eafa3ee
Compare
|
Here's proof I signed the CLA:
Also I fixed the build failures I had & |
|
IMO this is not the right approach. Hiding "incompatibilities" behind non-default-on flags is usually not intuitive behavior. If we were failing for those, and then could say "re-run with |
libredex/Trace.h
Outdated
| @@ -133,7 +134,7 @@ | |||
| TM(VERIFY) \ | |||
| TM(VIRT) \ | |||
| TM(VM) \ | |||
| TM(VMERGE) \ | |||
| TM(VMERGE) | |||
There was a problem hiding this comment.
The comment didn't make your linter ignore the "trailing" \? It's there intentionally to keep changes smaller in case there's an addition at the end :-)
There was a problem hiding this comment.
Oh that makes sense! I think I might've mistakenly removed this after I rebased... sorry about that 😅
libredex/Trace.cpp
Outdated
| @@ -129,7 +129,7 @@ struct Tracer { | |||
| for (const char* tok = strtok(tracespec, sep); tok != nullptr; | |||
| tok = strtok(nullptr, sep)) { | |||
| auto level = strtol(tok, nullptr, 10); | |||
| if (level) { | |||
| if (level || *tok == '0') { | |||
There was a problem hiding this comment.
Shouldn't this rather be strcmp? This is somewhat hackish. :-)
There was a problem hiding this comment.
True, this should be strcmp!
On another note, hackish code from @danzimm ? Who would've expected that! (cc @wsanville )
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Ahh, that's a good point @agampe ! Thanks for giving this a thoughtful review. I'll go ahead and default the PROGUARD trace to |
| @@ -91,6 +91,7 @@ | |||
| TM(PGR) \ | |||
| TM(PM) \ | |||
| TM(POST_LOWERING) \ | |||
| TM(PROGUARD) \ | |||
There was a problem hiding this comment.
Some of the existing code uses PGR to denote ProGuard trace statement, probably makes sense to reuse that?
There was a problem hiding this comment.
Whoa didn't realize that's what that stood for! Thanks for pointing that out! I'll get a new rev up soon!
If a user has a proguard file which uses a lot of unimplemented features in redex then there's a lot of
stderrspew. This PR changes thosestderrlogs toTRACElogs so we can hide that output.Additionally this PR changes the
TRACEsystem to allow a user to specify0for a given subsystem. E.g. now one can run redex withTRACE=1;PROGUARD:0to enable all traces at level 1, but disabling proguard traces. I can put this in another PR, but it seems so small and tangentially relevant to this PR that I put it in here (also I figured 3 PRs is enough for today)