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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
