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

[Mads Ravn via Phabricator via cfe-commits]

madsravn added inline comments.


================
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+  auto Diag = [=]() {
+    std::string Message = ReplaceMessage;
----------------
aaron.ballman wrote:
> madsravn wrote:
> > aaron.ballman wrote:
> > > madsravn wrote:
> > > > aaron.ballman wrote:
> > > > > Is there a reason this needs to capture everything by copy? Also, no need for the empty parens. Actually, is the lambda even necessary at all?
> > > > Is it OK to capture by reference then? Or how do we want it in llvm? 
> > > > 
> > > > We need the lambda, because first I need to create the diag with a message based on the count of arguments and then I need to find fixits based on the same count. Example: 
> > > > 
> > > > 
> > > > ```
> > > > string message = "Message for 2 arguments";
> > > > if(argumentCount == 3) {
> > > >   message = "Message for 3 arguments";
> > > > }
> > > > auto Diag = diag(startLoc(), message);
> > > > if(argumentCount == 3) {
> > > >   Diag << FixitHint::FixForThreeArguments();
> > > > } else {
> > > >   Diag << FixitHint::FixForTwoArguments();
> > > > }
> > > > ```
> > > > 
> > > > So the idea with the lambda is to avoid doing the same if-statement twice. 
> > > But you call the lambda immediately rather than store it and reuse it? It seems like you should be able to hoist a `DiagnosticBuilder` variable outside of the if statement and skip the lambda entirely.
> > I am not sure what you mean by this. Can you elaborate? Can you give a short example how I would hoist a `DiagnosticBuilder` out?
> > 
> > I think I tried something like that, but it was not an option. 
> It's entirely possible I'm missing something (I'm distracted with meetings this week), but I was envisioning:
> ```
> DiagnosticBuilder Diag;
> if (MatchedCallExpr->getNumArgs() == 3) {
>   Diag =
>       diag(MatchedCallExpr->getLocStart(),
>            "'std::random_shuffle' has been removed in C++17; use "
>            "'std::shuffle' and an alternative random mechanism instead");
>   Diag << FixItHint::CreateReplacement(
>       MatchedArgumentThree->getSourceRange(),
>       "std::mt19937(std::random_device()())");
> } else {
>   Diag = diag(MatchedCallExpr->getLocStart(),
>                     "'std::random_shuffle' has been removed in C++17; use "
>                     "'std::shuffle' instead");
>   Diag << FixItHint::CreateInsertion(
>       MatchedCallExpr->getRParenLoc(),
>       ", std::mt19937(std::random_device()())");
> }
> ```
The constructor for `DiagnosticBuilder` is private. So I cannot do that. The idea had crossed my mind, but I think the lambda expression is nicer to look at. 

Should I investigate if there is another way to hoist the `DiagnosticBuilder` out, like using `diag()` to make a dummy `DiagnosticBuilder` outside and then use the copy constructor to assign inside the if-statement? Or can we live with the lambda expression? 


https://reviews.llvm.org/D30158