[PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'

[Martin Böhme via cfe-commits]

mboehme marked 9 inline comments as done.
mboehme added a comment.

> > > 2. Also it would be good to make link in cppcoreguidelines.

> 

> > 

> 

> > 

> 

> > How exactly would I create such a "link"? Are you just thinking of a link in the documentation, or is there a way to have one clang-tidy check activate another (and is this what you're thinking of)?

> 

> 

> I am not sure if there is any other mechanism than just links in documentation.


I've taken a look, but I'm not sure where I would put this link. I could add another file that starts with "cppcoreguidelines-", but that would make it look as if an actual check with that name exists, and I'm not sure that's what we want. Do you have a suggestion?

> In the perfect word it would be nice to invoke this check using cppcoreguidelines-use-after-move also with some special options like Pedantic=1 (That would warn about any use after move, even after reinitialization - this is what cppcoreguidelines says)


Do you have a pointer to something in CppCoreGuidelines that says this explicitly?

The closest I've been able to find is in F.18: "Flag access to moved-from objects." It's not entirely clear here whether "access" is meant to include reinitialization.

However, other parts of CppCoreGuidelines seem to imply that it _is_ OK to reinitialize an object:

From ES.56:
"Usually, a std::move() is used as an argument to a && parameter. And after you do that, assume the object has been moved from (see C.64) and don't read its state again until you first set it to a new value."

And from C.64:
"Unless there is an exceptionally strong reason not to, make x = std::move(y); y = z; work with the conventional semantics."


================
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:29
@@ +28,3 @@
+/// Provides information about the evaluation order of (sub-)expressions within
+/// a CFGBlock.
+///
----------------
alexfh wrote:
> Please enclose inline code snippets in double backquotes (doxygen supports markdown to a certain degree).
Shouldn't those be single backquotes? (See https://www.stack.nl/~dimitri/doxygen/manual/markdown.html)

Done with single backquotes.

================
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:46
@@ +45,3 @@
+///   API),
+/// - Removing the dependency of SequenceChecker on Sema, and
+/// - (Probably) modifying SequenceChecker to make it suitable to be used in
----------------
alexfh wrote:
> Does it look feasible?
At least it's not something that I feel I would be able to do without breaking things -- I'm not familiar enough with SequenceChecker for that.

================
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:135
@@ +134,3 @@
+class UseAfterMoveFinder {
+public:
+  UseAfterMoveFinder(ASTContext *TheContext);
----------------
alexfh wrote:
> It's definitely subjective, but I don't think it's scary. And the size of the file doesn't seem to be an issue yet. IMO, a reason to move this to a header would appear once this class is needed elsewhere. 
Leaving it here for now.

================
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:217
@@ +216,3 @@
+  for (const auto &SyntheticStmt : TheCFG->synthetic_stmts()) {
+    SyntheticStmtSourceMap[SyntheticStmt.first] = SyntheticStmt.second;
+  }
----------------
I've added CFG::synthetic_stmts() in D23842.

================
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:436
@@ +435,3 @@
+        return true;
+    }
+  }
----------------
Added CFGBlock::succs() in D23842. This looks much nicer now!

================
Comment at: test/clang-tidy/misc-use-after-move.cpp:505-506
@@ +504,4 @@
+void noreturnDestructor() {
+  A a;
+  // The while loop in the ASSERT() would ordinarily have the potential to cause
+  // a use-after-move because the second iteration of the loop would be using a
----------------
Noted for a future addition. I'd like to get this patch in though without further expanding the functionality (it's already pretty big...).


https://reviews.llvm.org/D23353