[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
alexfh added a comment.
One late comment.
Comment at: clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.cpp:154-155
+const Stmt *ExprSequence::resolveSyntheticStmt(const Stmt *S) const {
+ if (SyntheticStmtSourceMap.count(S))
+return SyntheticStmtSourceMap.lookup(S);
+ return S;
Use `.find` and store the iterator to avoid duplicate lookups.
Repository:
rL LLVM
https://reviews.llvm.org/D27700
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
This revision was automatically updated to reflect the committed changes.
Closed by commit rL290489: [clang-tidy] refactor ExprSequence out of
use-after-move check (authored by mnbvmar).
Changed prior to commit:
https://reviews.llvm.org/D27700?vs=81732=82440#toc
Repository:
rL LLVM
https://reviews.llvm.org/D27700
Files:
clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp
clang-tools-extra/trunk/clang-tidy/utils/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.cpp
clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.h
Index: clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.h
===
--- clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.h
+++ clang-tools-extra/trunk/clang-tidy/utils/ExprSequence.h
@@ -0,0 +1,124 @@
+//===- ExprSequence.h - clang-tidy ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRSEQUENCE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRSEQUENCE_H
+
+#include "clang/Analysis/CFG.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+/// Provides information about the evaluation order of (sub-)expressions within
+/// a `CFGBlock`.
+///
+/// While a `CFGBlock` does contain individual `CFGElement`s for some
+/// sub-expressions, the order in which those `CFGElement`s appear reflects
+/// only one possible order in which the sub-expressions may be evaluated.
+/// However, we want to warn if any of the potential evaluation orders can lead
+/// to a use-after-move, not just the one contained in the `CFGBlock`.
+///
+/// This class implements only a simplified version of the C++ sequencing
+/// rules. The main limitation is that we do not distinguish between value
+/// computation and side effect -- see the "Implementation" section for more
+/// details.
+///
+/// Note: `SequenceChecker` from SemaChecking.cpp does a similar job (and much
+/// more thoroughly), but using it would require
+/// - Pulling `SequenceChecker` out into a header file (i.e. making it part of
+/// the API),
+/// - Removing the dependency of `SequenceChecker` on `Sema`, and
+/// - (Probably) modifying `SequenceChecker` to make it suitable to be used in
+/// this context.
+/// For the moment, it seems preferable to re-implement our own version of
+/// sequence checking that is special-cased to what we need here.
+///
+/// Implementation
+/// --
+///
+/// `ExprSequence` uses two types of sequencing edges between nodes in the AST:
+///
+/// - Every `Stmt` is assumed to be sequenced after its children. This is
+/// overly optimistic because the standard only states that value computations
+/// of operands are sequenced before the value computation of the operator,
+/// making no guarantees about side effects (in general).
+///
+/// For our purposes, this rule is sufficient, however, because this check is
+/// interested in operations on objects, which are generally performed through
+/// function calls (whether explicit and implicit). Function calls guarantee
+/// that the value computations and side effects for all function arguments
+/// are sequenced before the execution of the function.
+///
+/// - In addition, some `Stmt`s are known to be sequenced before or after
+/// their siblings. For example, the `Stmt`s that make up a `CompoundStmt`are
+/// all sequenced relative to each other. The function
+/// `getSequenceSuccessor()` implements these sequencing rules.
+class ExprSequence {
+public:
+ /// Initializes this `ExprSequence` with sequence information for the given
+ /// `CFG`.
+ ExprSequence(const CFG *TheCFG, ASTContext *TheContext);
+
+ /// Returns whether \p Before is sequenced before \p After.
+ bool inSequence(const Stmt *Before, const Stmt *After) const;
+
+ /// Returns whether \p After can potentially be evaluated after \p Before.
+ /// This is exactly equivalent to `!inSequence(After, Before)` but makes some
+ /// conditions read more naturally.
+ bool potentiallyAfter(const Stmt *After, const Stmt *Before) const;
+
+private:
+ // Returns the sibling of \p S (if any) that is directly sequenced after \p S,
+ // or nullptr if no such sibling exists. For example, if \p S is the child of
+ // a `CompoundStmt`, this would return the Stmt that directly follows \p S in
+ // the `CompoundStmt`.
+ //
+ // As the sequencing of many constructs that change control flow is already
+ // encoded in the `CFG`, this function only implements the sequencing rules
+ // for those
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
mnbvmar marked 9 inline comments as done. mnbvmar added inline comments. Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:18 using namespace clang::ast_matchers; +using namespace clang::tidy::utils; + mboehme wrote: > Prazek wrote: > > Prazek wrote: > > > I don't think it is required > > ok I guess I am wrong > I would suggest instead adding an explicit "utils::" qualifier -- it's only > needed in two places anyway. I don't feel strongly about this though. It's actually needed in more places (I think at least four). I feel like leaving it here. https://reviews.llvm.org/D27700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
mnbvmar updated this revision to Diff 81732.
mnbvmar marked 2 inline comments as done.
mnbvmar added a comment.
Minor changes, according to the request.
https://reviews.llvm.org/D27700
Files:
clang-tidy/misc/UseAfterMoveCheck.cpp
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/ExprSequence.cpp
clang-tidy/utils/ExprSequence.h
Index: clang-tidy/utils/ExprSequence.h
===
--- /dev/null
+++ clang-tidy/utils/ExprSequence.h
@@ -0,0 +1,124 @@
+//===- ExprSequence.h - clang-tidy ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRSEQUENCE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_EXPRSEQUENCE_H
+
+#include "clang/Analysis/CFG.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+/// Provides information about the evaluation order of (sub-)expressions within
+/// a `CFGBlock`.
+///
+/// While a `CFGBlock` does contain individual `CFGElement`s for some
+/// sub-expressions, the order in which those `CFGElement`s appear reflects
+/// only one possible order in which the sub-expressions may be evaluated.
+/// However, we want to warn if any of the potential evaluation orders can lead
+/// to a use-after-move, not just the one contained in the `CFGBlock`.
+///
+/// This class implements only a simplified version of the C++ sequencing
+/// rules. The main limitation is that we do not distinguish between value
+/// computation and side effect -- see the "Implementation" section for more
+/// details.
+///
+/// Note: `SequenceChecker` from SemaChecking.cpp does a similar job (and much
+/// more thoroughly), but using it would require
+/// - Pulling `SequenceChecker` out into a header file (i.e. making it part of
+/// the API),
+/// - Removing the dependency of `SequenceChecker` on `Sema`, and
+/// - (Probably) modifying `SequenceChecker` to make it suitable to be used in
+/// this context.
+/// For the moment, it seems preferable to re-implement our own version of
+/// sequence checking that is special-cased to what we need here.
+///
+/// Implementation
+/// --
+///
+/// `ExprSequence` uses two types of sequencing edges between nodes in the AST:
+///
+/// - Every `Stmt` is assumed to be sequenced after its children. This is
+/// overly optimistic because the standard only states that value computations
+/// of operands are sequenced before the value computation of the operator,
+/// making no guarantees about side effects (in general).
+///
+/// For our purposes, this rule is sufficient, however, because this check is
+/// interested in operations on objects, which are generally performed through
+/// function calls (whether explicit and implicit). Function calls guarantee
+/// that the value computations and side effects for all function arguments
+/// are sequenced before the execution of the function.
+///
+/// - In addition, some `Stmt`s are known to be sequenced before or after
+/// their siblings. For example, the `Stmt`s that make up a `CompoundStmt`are
+/// all sequenced relative to each other. The function
+/// `getSequenceSuccessor()` implements these sequencing rules.
+class ExprSequence {
+public:
+ /// Initializes this `ExprSequence` with sequence information for the given
+ /// `CFG`.
+ ExprSequence(const CFG *TheCFG, ASTContext *TheContext);
+
+ /// Returns whether \p Before is sequenced before \p After.
+ bool inSequence(const Stmt *Before, const Stmt *After) const;
+
+ /// Returns whether \p After can potentially be evaluated after \p Before.
+ /// This is exactly equivalent to `!inSequence(After, Before)` but makes some
+ /// conditions read more naturally.
+ bool potentiallyAfter(const Stmt *After, const Stmt *Before) const;
+
+private:
+ // Returns the sibling of \p S (if any) that is directly sequenced after \p S,
+ // or nullptr if no such sibling exists. For example, if \p S is the child of
+ // a `CompoundStmt`, this would return the Stmt that directly follows \p S in
+ // the `CompoundStmt`.
+ //
+ // As the sequencing of many constructs that change control flow is already
+ // encoded in the `CFG`, this function only implements the sequencing rules
+ // for those constructs where sequencing cannot be inferred from the `CFG`.
+ const Stmt *getSequenceSuccessor(const Stmt *S) const;
+
+ const Stmt *resolveSyntheticStmt(const Stmt *S) const;
+
+ ASTContext *Context;
+
+ llvm::DenseMap SyntheticStmtSourceMap;
+};
+
+/// Maps `Stmt`s to the `CFGBlock` that contains them. Some `Stmt`s may be
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
mboehme accepted this revision.
mboehme added a comment.
This revision is now accepted and ready to land.
LG apart from minor comments by others and me
Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:18
using namespace clang::ast_matchers;
+using namespace clang::tidy::utils;
+
Prazek wrote:
> Prazek wrote:
> > I don't think it is required
> ok I guess I am wrong
I would suggest instead adding an explicit "utils::" qualifier -- it's only
needed in two places anyway. I don't feel strongly about this though.
Comment at: clang-tidy/utils/ExprSequence.cpp:52
+
+bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
+ ASTContext *Context) {
Prazek wrote:
> staronj wrote:
> > Shouldn't isDescendantOrEqual be static or in inline namespace?
> Goot catch. I guess putting it with getParentStmts into anonymous namespace
> is the best solution.
> btw inline namespace is not the same as anonymous namespace :)
Thanks for the catch -- this was already a bug in the original code...
Comment at: clang-tidy/utils/ExprSequence.cpp:179
+
+
+}
Double newline
Comment at: clang-tidy/utils/ExprSequence.h:120
+
+
+}
Double newline
https://reviews.llvm.org/D27700
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
Prazek added inline comments.
Comment at: clang-tidy/utils/ExprSequence.cpp:52
+
+bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
+ ASTContext *Context) {
staronj wrote:
> Shouldn't isDescendantOrEqual be static or in inline namespace?
Goot catch. I guess putting it with getParentStmts into anonymous namespace is
the best solution.
btw inline namespace is not the same as anonymous namespace :)
https://reviews.llvm.org/D27700
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
staronj added inline comments.
Comment at: clang-tidy/utils/ExprSequence.cpp:52
+
+bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
+ ASTContext *Context) {
Shouldn't isDescendantOrEqual be static or in inline namespace?
https://reviews.llvm.org/D27700
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
alexfh added inline comments. Comment at: clang-tidy/utils/ExprSequence.cpp:154 +return SyntheticStmtSourceMap.lookup(S); + else +return S; Prazek wrote: > alexfh wrote: > > nit: No `else` after return, please. > Not sure if he should change it in this patch - it is just move of this class > to different file, so I am not sure if it is good do introduce small changes > to it now. > I guess pushin NFC patch with this after would be good solution For me it's usually easier to fix than to postpone to a different patch. Feel free to do either. https://reviews.llvm.org/D27700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
Prazek added inline comments. Comment at: clang-tidy/utils/ExprSequence.cpp:154 +return SyntheticStmtSourceMap.lookup(S); + else +return S; alexfh wrote: > nit: No `else` after return, please. Not sure if he should change it in this patch - it is just move of this class to different file, so I am not sure if it is good do introduce small changes to it now. I guess pushin NFC patch with this after would be good solution https://reviews.llvm.org/D27700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
Prazek added inline comments. Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:18 using namespace clang::ast_matchers; +using namespace clang::tidy::utils; + Prazek wrote: > I don't think it is required ok I guess I am wrong https://reviews.llvm.org/D27700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27700: [clang-tidy] refactor ExprSequence out of misc-use-after-move check
Prazek added inline comments. Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:18 using namespace clang::ast_matchers; +using namespace clang::tidy::utils; + I don't think it is required Comment at: clang-tidy/utils/ExprSequence.cpp:180-182 +} +} +} same here Comment at: clang-tidy/utils/ExprSequence.h:121-123 +} +} +} Run clang-tidy llvm checks on this patch. These braces requires comments like // namespace clang https://reviews.llvm.org/D27700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
