[PATCH] D31308: [clang-tidy] new check readability-no-alternative-tokens

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 4 06:06:40 PDT 2017


aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/OperatorsRepresentationCheck.cpp:34
+
+  if (const auto *B = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
+    switch (B->getOpcode()) {
----------------
alexfh wrote:
> Eugene.Zelenko wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > mgehre wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > alexfh wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > I think this would make more sense lifted into an AST matcher -- there are bound to be a *lot* of binary operators, so letting the matcher memoize things is likely to give better performance.
> > > > > > > > Any reasons not to do this on the lexer level?
> > > > > > > Technical reasons? None.
> > > > > > > User-experience reasons? We wouldn't want this to be on by default (I don't think) and we usually don't implement off-by-default diagnostics in Clang. I think a case could be made for doing it in the Lexer if the performance is really that bad with a clang-tidy check and we couldn't improve it here, though.
> > > > > > Do I correctly understand that "doing this on lexer level" would mean to implement this as a warning directly into clang? If yes, would it be possible to generate fixits and have them possibly applied automatically (as it is the case for clang-tidy)?
> > > > > You are correct, that means implementing it as a warning in Clang. I believe you can still generate those fixits from lexer-level diagnostics, but have not verified it.
> > > > > 
> > > > > However, I don't think this diagnostic would be appropriate for Clang because it would have to be off by default.
> > > > Actually, I was thinking about changing this clang-tidy check to analyze token stream somehow (probably by handling `PPCallbacks` to detect ranges that need to be re-lexed) instead of matching the AST. I didn't intend to propose a new Clang warning (but it looks like the wording was misleading).
> > > There is some value in that -- it means we could support C, for instance. I'm not certain how easy or hard it would be, but suspect it's reasonable. However, in C, there's still the problem of the include file that introduces those macros. Do we have facilities to remove an include in clang-tidy?
> > Yes, it may make sense in C, but parameter for map from macro name to operator is needed.
> > 
> > Product I'm working for, with long history, had alternative operator presentation implemented in C.
> Changing macro-based "alternative tokens" to the corresponding operators can also be formulated in the terms of expanding a set of macros, which should work with any LangOpts and arbitrary alternative operator names.
> 
> > However, in C, there's still the problem of the include file that introduces those macros. 
> 
> Sounds like a generic "remove unused includes" problem, since we need to analyze whether the header is needed for anything else. In particular, even if the use of the header is limited to the alternative representations of operators, we can't remove the include until we replace all these macros. Clang-tidy doesn't provide any support for transactional fixes and dependencies between fixes.
> Sounds like a generic "remove unused includes" problem, since we need to analyze whether the header is needed for anything else. In particular, even if the use of the header is limited to the alternative representations of operators, we can't remove the include until we replace all these macros. Clang-tidy doesn't provide any support for transactional fixes and dependencies between fixes.

That's a good point. I also don't think an unused include is sufficiently awful to block this.


https://reviews.llvm.org/D31308





More information about the cfe-commits mailing list