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:
> > > > > 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?
> Ah, okay, that was the bit I was missing. Thank you for being patient. I
> think the lambda (with the reference capture) is fine as-is.
> Thank you for being patient.
Right back at you. We are working towards the same goal after all :)
For future reference: Should I try to avoid lambda expressions like this?
https://reviews.llvm.org/D30158
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
