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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
