[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro marked an inline comment as done.
djtodoro added a comment.

> I'm not quite sure what this differential is about, but i feel like mentioning ExprMutationAnalyzer lib in clang-tidy / clang-tools-extra.



>> Alternatively perhaps you could re-use getMemoryLocation() from D57660 <https://reviews.llvm.org/D57660>. It would handle for you members, references, structured bindings +more in a relatively self-contained code.

@lebedev.ri, @riccibruno Thanks for your advices! We'll check this also.



================
Comment at: lib/Sema/SemaExpr.cpp:11301
+    EmitArgumentsValueModification(E);
+
   SourceLocation OrigLoc = Loc;
----------------
lebedev.ri wrote:
> riccibruno wrote:
> > Comments:
> > 
> > 1. Shouldn't you mark the variable to be modified only if `CheckForModifiableLvalue` returns true ?
> > 
> > 2. I think that you need to handle way more than just member expressions. For example are you handling `(x, y)` (comma operator) ? But hard-coding every cases here is probably not ideal. It would be nice if there was already some code somewhere that could help you do this.
> > 
> > 3. I believe that a `MemberExpr` has always a base. Similarly `DeclRefExpr`s always have a referenced declaration (so you can omit the `if`).
> I'm not quite sure what this differential is about, but i feel like mentioning ExprMutationAnalyzer lib in clang-tidy / clang-tools-extra.
@riccibruno Please find inlined replies:

>1. Shouldn't you mark the variable to be modified only if CheckForModifiableLvalue returns true ?

Hmm, we should avoid marking variables modification if this emits an error. But, we should emit it if `CheckForModifiableLvalue ` returns `false`, since in the case of returning `true` an error will be emitted.

>2. I think that you need to handle way more than just member expressions. For example are you handling (x, y) (comma operator) ? But hard-coding every cases here is probably not ideal. It would be nice if there was already some code somewhere that could help you do this.

Hard coding all cases is not good idea. I agree. Since we are only looking for declaration references, we could make just a function that traverses any `Expr` and find declaration ref.

>3. I believe that a MemberExpr has always a base. Similarly DeclRefExprs always have a referenced declaration (so you can omit the if).

I think you are right. Thanks!




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58035/new/

https://reviews.llvm.org/D58035