aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
I think this check is in good shape for the initial commit. Additional functionality can be added incrementally. ================ Comment at: test/clang-tidy/readability-redundant-preprocessor.cpp:53 +// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor] +#if FOO == 3 + 1 +// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here ---------------- vmiklos wrote: > aaron.ballman wrote: > > I didn't describe my test scenario very well -- can you change this line to > > FOO == 4 but leave the preceding conditional check as FOO == 3 + 1? The > > goal is to test that this isn't a token-by-token check, but uses the > > symbolic values instead. > That doesn't work at the moment, since `PreprocessorEntry::Condition` is just > a string, so `3 + 1` won't equal to `4`. I think we discussed this above > already, and you said: > > > I think it requires manual work. There's a FIXME in `PPCallbacks::If()` to > > pass a list/tree of tokens that would make implementing this reasonable. > > I'd say it's fine as a follow-up patch. > > But later you wrote: > > > I agree that it's more complex, but that's why I was asking for it -- I > > don't think the design you have currently will extend for these sort of > > cases, and I was hoping to cover more utility with the check to hopefully > > shake out those forward-looking design considerations. As it stands, I feel > > like this check is a bit too simplistic to have much value, but if you > > covered some of these other cases, it would alleviate that worry for me. > > My hope is that the check with its current "feature set" already has some > value, but let me know if I'm too optimistic. :-) > > That being said, if I want to support simple cases like "realize that `3 + 1` > and `4` is the same" -- do you imagine that would be manually handled in this > check or there is already some reusable building block in the codebase to > evaluate if two expressions have the same integer value? I guess doing this > manually is a lot of work, e.g. the answer for `FOO + 4` would be "it > depends", so that should not equal to `8`, even if FOO happens to be `4`, etc. That's a good point -- sorry about the mistake on my suggestion that this test should pass, I had a temporary lapse. :-) https://reviews.llvm.org/D54349 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
