[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
alexfh added a comment. In https://reviews.llvm.org/D25406#633892, @malcolm.parsons wrote: > I'd prefer a -format option to clang-tidy. Exactly. https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
malcolm.parsons abandoned this revision. malcolm.parsons added a comment. I'd prefer a -format option to clang-tidy. https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
malcolm.parsons added a comment. ping @alexfh. https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseAutoCheck.cpp:378 + Lexer::getLocForEndOfToken(Range.getEnd(), 0, SM, Context->getLangOpts()); + bool Whitespace = isWhitespace(*FullSourceLoc(Next, SM).getCharacterData()); + malcolm.parsons wrote: > aaron.ballman wrote: > > malcolm.parsons wrote: > > > aaron.ballman wrote: > > > > malcolm.parsons wrote: > > > > > aaron.ballman wrote: > > > > > > Oye, this is deceptively expensive because you now have to go back > > > > > > to the actual source file for this information. That source file > > > > > > may live on a network share somewhere, for instance. > > > > > > > > > > > > Can you use `Token::hasLeadingSpace()` instead? > > > > > > > > > > > > Also, doesn't this still need to care about the `RemoveStars` > > > > > > option? > > > > > Where would I get a Token from? > > > > Hrm, might not be as trivial as I was hoping (I thought we had a way to > > > > go from a `SourceLocation` back to a `Token`, but I'm not seeing one > > > > off-hand). Regardless, I worry about the expense of going all the way > > > > back to the source for this. > > > > > > > > @alexfh -- should this functionality be a part of a more general "we've > > > > made a fixit, now make it not look ugly?" pass? At least then, if we go > > > > back to the source, we can do it in a more controlled manner and > > > > hopefully get some performance back from that. > > > `LineIsMarkedWithNOLINT` is going to read the source anyway, so I don't > > > see any additional expense. > > The additional expense comes from many checks needing similar functionality > > -- generally, fixit replacements are going to require formatting > > modifications of some sort. It's better to handle all of those in the same > > place and transparently rather than have every check with a fixit manually > > handle formatting. Additionally, this means we can go back to the source as > > infrequently as possible. > I ran strace -c on clang-tidy on several real world source files that > modernize-use-auto warns about before and after this change and the counts > for read, write, stat, open, close, openat and fstat syscalls were all > unchanged. > The expense is exactly zero. > It will still be zero when multiplied by many checks. > There is no performance issue here. > Do you have any other issues with this change? Thank you for running strace to see what the behavior is. From what I understand, this is hard to judge because it depends on when the SourceManager elects to drop its underlying memory buffer (if at all). We definitely try to avoid going back to source when possible in Clang; I think clang-tidy should take a similar policy. I don't think the fix you have is incorrect. I am, however, not convinced this is the best way to solve the problem because we're playing whack-a-mole with fixing replacements already, and this is one more instance of such need. @alexfh had suggested (in a different review thread about a similar need) that we solve this through a convenient bottleneck (such as the diagnostics engine) that basically runs clang-format over replaced text, handles extraneous commas, etc and I would prefer to hear if efforts are being made/planned for that approach before committing another one-off solution. The extra space included in the current behavior is not a correctness issue, so I'm not too worried about getting this fix in immediately if there's work in progress on a better approach. https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
malcolm.parsons added inline comments. Comment at: clang-tidy/modernize/UseAutoCheck.cpp:378 + Lexer::getLocForEndOfToken(Range.getEnd(), 0, SM, Context->getLangOpts()); + bool Whitespace = isWhitespace(*FullSourceLoc(Next, SM).getCharacterData()); + aaron.ballman wrote: > malcolm.parsons wrote: > > aaron.ballman wrote: > > > malcolm.parsons wrote: > > > > aaron.ballman wrote: > > > > > Oye, this is deceptively expensive because you now have to go back to > > > > > the actual source file for this information. That source file may > > > > > live on a network share somewhere, for instance. > > > > > > > > > > Can you use `Token::hasLeadingSpace()` instead? > > > > > > > > > > Also, doesn't this still need to care about the `RemoveStars` option? > > > > Where would I get a Token from? > > > Hrm, might not be as trivial as I was hoping (I thought we had a way to > > > go from a `SourceLocation` back to a `Token`, but I'm not seeing one > > > off-hand). Regardless, I worry about the expense of going all the way > > > back to the source for this. > > > > > > @alexfh -- should this functionality be a part of a more general "we've > > > made a fixit, now make it not look ugly?" pass? At least then, if we go > > > back to the source, we can do it in a more controlled manner and > > > hopefully get some performance back from that. > > `LineIsMarkedWithNOLINT` is going to read the source anyway, so I don't see > > any additional expense. > The additional expense comes from many checks needing similar functionality > -- generally, fixit replacements are going to require formatting > modifications of some sort. It's better to handle all of those in the same > place and transparently rather than have every check with a fixit manually > handle formatting. Additionally, this means we can go back to the source as > infrequently as possible. I ran strace -c on clang-tidy on several real world source files that modernize-use-auto warns about before and after this change and the counts for read, write, stat, open, close, openat and fstat syscalls were all unchanged. The expense is exactly zero. It will still be zero when multiplied by many checks. There is no performance issue here. Do you have any other issues with this change? https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseAutoCheck.cpp:378 + Lexer::getLocForEndOfToken(Range.getEnd(), 0, SM, Context->getLangOpts()); + bool Whitespace = isWhitespace(*FullSourceLoc(Next, SM).getCharacterData()); + malcolm.parsons wrote: > aaron.ballman wrote: > > malcolm.parsons wrote: > > > aaron.ballman wrote: > > > > Oye, this is deceptively expensive because you now have to go back to > > > > the actual source file for this information. That source file may live > > > > on a network share somewhere, for instance. > > > > > > > > Can you use `Token::hasLeadingSpace()` instead? > > > > > > > > Also, doesn't this still need to care about the `RemoveStars` option? > > > Where would I get a Token from? > > Hrm, might not be as trivial as I was hoping (I thought we had a way to go > > from a `SourceLocation` back to a `Token`, but I'm not seeing one > > off-hand). Regardless, I worry about the expense of going all the way back > > to the source for this. > > > > @alexfh -- should this functionality be a part of a more general "we've > > made a fixit, now make it not look ugly?" pass? At least then, if we go > > back to the source, we can do it in a more controlled manner and hopefully > > get some performance back from that. > `LineIsMarkedWithNOLINT` is going to read the source anyway, so I don't see > any additional expense. The additional expense comes from many checks needing similar functionality -- generally, fixit replacements are going to require formatting modifications of some sort. It's better to handle all of those in the same place and transparently rather than have every check with a fixit manually handle formatting. Additionally, this means we can go back to the source as infrequently as possible. https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
malcolm.parsons added inline comments. Comment at: clang-tidy/modernize/UseAutoCheck.cpp:378 + Lexer::getLocForEndOfToken(Range.getEnd(), 0, SM, Context->getLangOpts()); + bool Whitespace = isWhitespace(*FullSourceLoc(Next, SM).getCharacterData()); + aaron.ballman wrote: > malcolm.parsons wrote: > > aaron.ballman wrote: > > > Oye, this is deceptively expensive because you now have to go back to the > > > actual source file for this information. That source file may live on a > > > network share somewhere, for instance. > > > > > > Can you use `Token::hasLeadingSpace()` instead? > > > > > > Also, doesn't this still need to care about the `RemoveStars` option? > > Where would I get a Token from? > Hrm, might not be as trivial as I was hoping (I thought we had a way to go > from a `SourceLocation` back to a `Token`, but I'm not seeing one off-hand). > Regardless, I worry about the expense of going all the way back to the source > for this. > > @alexfh -- should this functionality be a part of a more general "we've made > a fixit, now make it not look ugly?" pass? At least then, if we go back to > the source, we can do it in a more controlled manner and hopefully get some > performance back from that. `LineIsMarkedWithNOLINT` is going to read the source anyway, so I don't see any additional expense. https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseAutoCheck.cpp:378 + Lexer::getLocForEndOfToken(Range.getEnd(), 0, SM, Context->getLangOpts()); + bool Whitespace = isWhitespace(*FullSourceLoc(Next, SM).getCharacterData()); + malcolm.parsons wrote: > aaron.ballman wrote: > > Oye, this is deceptively expensive because you now have to go back to the > > actual source file for this information. That source file may live on a > > network share somewhere, for instance. > > > > Can you use `Token::hasLeadingSpace()` instead? > > > > Also, doesn't this still need to care about the `RemoveStars` option? > Where would I get a Token from? Hrm, might not be as trivial as I was hoping (I thought we had a way to go from a `SourceLocation` back to a `Token`, but I'm not seeing one off-hand). Regardless, I worry about the expense of going all the way back to the source for this. @alexfh -- should this functionality be a part of a more general "we've made a fixit, now make it not look ugly?" pass? At least then, if we go back to the source, we can do it in a more controlled manner and hopefully get some performance back from that. https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
malcolm.parsons added inline comments. Comment at: clang-tidy/modernize/UseAutoCheck.cpp:378 + Lexer::getLocForEndOfToken(Range.getEnd(), 0, SM, Context->getLangOpts()); + bool Whitespace = isWhitespace(*FullSourceLoc(Next, SM).getCharacterData()); + aaron.ballman wrote: > Oye, this is deceptively expensive because you now have to go back to the > actual source file for this information. That source file may live on a > network share somewhere, for instance. > > Can you use `Token::hasLeadingSpace()` instead? > > Also, doesn't this still need to care about the `RemoveStars` option? Where would I get a Token from? https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25406: Fix doubled whitespace in modernize-use-auto fixit
aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseAutoCheck.cpp:378 + Lexer::getLocForEndOfToken(Range.getEnd(), 0, SM, Context->getLangOpts()); + bool Whitespace = isWhitespace(*FullSourceLoc(Next, SM).getCharacterData()); + Oye, this is deceptively expensive because you now have to go back to the actual source file for this information. That source file may live on a network share somewhere, for instance. Can you use `Token::hasLeadingSpace()` instead? Also, doesn't this still need to care about the `RemoveStars` option? https://reviews.llvm.org/D25406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
