[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:30
+    " The old user defined 'RandomFunction' is not usable for 'shuffle'. You "
+    "need to "
+    "make additional changes if you want a specific random function.";
----------------
madsravn wrote:
> xazax.hun wrote:
> > Maybe it would be worth to reflow this literal.
> It seems a little weird, but this is the result of clang-format. 
I usually just make it one big line and than clang format will do a better reflow. By default it do not remove linebreaks, even if it could improve the layout. 


================
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:70
+  llvm::raw_string_ostream Stream(Buffer);
+  DRef->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+  StringRef StrRef(Stream.str());
----------------
madsravn wrote:
> xazax.hun wrote:
> > What about accessing the buffer of the source file instead of pretty printing?
> How would you do this? printPretty was the best that I could find fitting my needs. If you have something better fitting, please share :)
I was thinking about something like getSourceText of Lexer. 


================
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:81
+  Stream << "shuffle(";
+  FirstArgument->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+  Stream << ", ";
----------------
madsravn wrote:
> xazax.hun wrote:
> > Wouldn't just using the original source range of the argument work?
> > What about preserving the comments next to the arguments? 
> I am not sure what you mean about the original source range. Can I just put that onto the Stream? 
> 
> Do you have an idea for the comments? Do you mean something like
> 
> 
> ```
> std::random_shuffle(
>    vec.begin(), // Comment
>    vec.end() // Comment
> );
> ```
Or even:


```
std::random_shuffle(vec.begin(), /*inlinecomment*/ vec.end());
```


================
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:101
+
+  auto Diag = diag(MatchedCallExpr->getLocStart(), Message);
+  Diag << FixItHint::CreateRemoval(MatchedCallExpr->getSourceRange());
----------------
madsravn wrote:
> xazax.hun wrote:
> > You do not want to do fixits for code that is the result of macro expansions. 
> Why not? And how would I fix that? 
Because that might cause the code not to compile when the macro is expanded in a different context. It is a conservative approach but other tidy checkers usually do this as well. You can use the isMacroID() method of the source locations. 


https://reviews.llvm.org/D30158