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