-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[clang][dataflow] Add matchers for smart pointer accessors to be cached #120102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is part 1 of caching for smart pointer accessors, building on top of the CachedConstAccessorsLattice, which caches "normal" accessors. Smart pointer accessors are a bit different in that they may: - have aliases to access the same underlying data (but potentially returning slightly different types like `&` vs `*`). Within a "checked" sequence users may mix uses of the different aliases and the check should apply to any of the spellings. - may have non-const overloads in addition to the const version, where the non-const doesn't actually modify the container Part 2 will follow and add transfer functions utilities. It will also add a user UncheckedOptionalAccessModel. We'd seen false positives when nesting StatusOr<optional<T>> and optional<StatusOr<T>>, etc. which this can help address.
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Jan Voung (jvoung) ChangesThis is part 1 of caching for smart pointer accessors, building on top Smart pointer accessors are a bit different in that they may:
Part 2 will follow and add transfer functions utilities. It will also Full diff: https://github.com/llvm/llvm-project/pull/120102.diff 5 Files Affected:
diff --git a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
new file mode 100644
index 00000000000000..1adb63af4e724b
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -0,0 +1,63 @@
+//===-- SmartPointerAccessorCaching.h ---------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines utilities to help cache accessors for smart pointer
+// like objects.
+//
+// These should be combined with CachedConstAccessorsLattice.
+// Beyond basic const accessors, smart pointers may have the following two
+// additional issues:
+//
+// 1) There may be multiple accessors for the same underlying object, e.g.
+// `operator->`, `operator*`, and `get`. Users may use a mixture of these
+// accessors, so the cache should unify them.
+//
+// 2) There may be non-const overloads of accessors. They are still safe to
+// cache, as they don't modify the container object.
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+
+#include <cassert>
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::dataflow {
+
+/// Matchers:
+/// For now, these match on any class with an `operator*` or `operator->`
+/// where the return types have a similar shape as std::unique_ptr
+/// and std::optional.
+///
+/// - `*` returns a reference to a type `T`
+/// - `->` returns a pointer to `T`
+/// - `get` returns a pointer to `T`
+/// - `value` returns a reference `T`
+///
+/// (1) The `T` should all match across the accessors (ignoring qualifiers).
+///
+/// (2) The specific accessor used in a call isn't required to be const,
+/// but the class must have a const overload of each accessor.
+///
+/// For now, we don't have customization to ignore certain classes.
+/// For example, if writing a ClangTidy check for `std::optional`, these
+/// would also match `std::optional`. In order to have special handling
+/// for `std::optional`, we assume the (Matcher, TransferFunction) case
+/// with custom handling is ordered early so that these generic cases
+/// do not trigger.
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar();
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorArrow();
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeValueMethodCall();
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeGetMethodCall();
+
+} // namespace clang::dataflow
+
+#endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
diff --git a/clang/lib/Analysis/FlowSensitive/CMakeLists.txt b/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
index 05cdaa7e27823d..0c30df8b4b194f 100644
--- a/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
@@ -10,6 +10,7 @@ add_clang_library(clangAnalysisFlowSensitive
Logger.cpp
RecordOps.cpp
SimplifyConstraints.cpp
+ SmartPointerAccessorCaching.cpp
Transfer.cpp
TypeErasedDataflowAnalysis.cpp
Value.cpp
diff --git a/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
new file mode 100644
index 00000000000000..967455322b57fe
--- /dev/null
+++ b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
@@ -0,0 +1,134 @@
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
+
+#include "clang/AST/CanonicalType.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/OperatorKinds.h"
+
+namespace clang::dataflow {
+
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxOperatorCallExpr;
+using ast_matchers::hasName;
+using ast_matchers::hasOverloadedOperatorName;
+using ast_matchers::ofClass;
+using ast_matchers::parameterCountIs;
+using ast_matchers::pointerType;
+using ast_matchers::referenceType;
+using ast_matchers::returns;
+
+bool hasSmartPointerClassShape(const CXXRecordDecl &RD, bool &HasGet,
+ bool &HasValue) {
+ // We may want to cache this search, but in current profiles it hasn't shown
+ // up as a hot spot (possibly because there aren't many hits, relatively).
+ bool HasArrow = false;
+ bool HasStar = false;
+ CanQualType StarReturnType, ArrowReturnType, GetReturnType, ValueReturnType;
+ for (const auto *MD : RD.methods()) {
+ // We only consider methods that are const and have zero parameters.
+ // It may be that there is a non-const overload for the method, but
+ // there should at least be a const overload as well.
+ if (!MD->isConst() || MD->getNumParams() != 0) {
+ continue;
+ }
+ if (MD->getOverloadedOperator() == OO_Star &&
+ MD->getReturnType()->isReferenceType()) {
+ HasStar = true;
+ StarReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+ } else if (MD->getOverloadedOperator() == OO_Arrow &&
+ MD->getReturnType()->isPointerType()) {
+ HasArrow = true;
+ ArrowReturnType =
+ MD->getReturnType()->getPointeeType()->getCanonicalTypeUnqualified();
+ } else {
+ IdentifierInfo *II = MD->getIdentifier();
+ if (II == nullptr)
+ continue;
+ if (II->isStr("get") && MD->getReturnType()->isPointerType()) {
+ HasGet = true;
+ GetReturnType = MD->getReturnType()
+ ->getPointeeType()
+ ->getCanonicalTypeUnqualified();
+ } else if (II->isStr("value") && MD->getReturnType()->isReferenceType()) {
+ HasValue = true;
+ ValueReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+ }
+ }
+ }
+
+ if (!HasStar || !HasArrow || StarReturnType != ArrowReturnType)
+ return false;
+ HasGet = HasGet && (GetReturnType == StarReturnType);
+ HasValue = HasValue && (ValueReturnType == StarReturnType);
+ return true;
+}
+
+} // namespace
+} // namespace clang::dataflow
+
+// AST_MATCHER macros create an "internal" namespace, so we put it in
+// its own anonymous namespace instead of in clang::dataflow.
+namespace {
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGet) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasGet;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasValue;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGetOrValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && (HasGet || HasValue);
+}
+
+} // namespace
+
+namespace clang::dataflow {
+
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar() {
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName("*"),
+ callee(cxxMethodDecl(parameterCountIs(0), returns(referenceType()),
+ ofClass(smartPointerClassWithGetOrValue()))));
+}
+
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorArrow() {
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName("->"),
+ callee(cxxMethodDecl(parameterCountIs(0), returns(pointerType()),
+ ofClass(smartPointerClassWithGetOrValue()))));
+}
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeValueMethodCall() {
+ return cxxMemberCallExpr(callee(
+ cxxMethodDecl(parameterCountIs(0), returns(referenceType()),
+ hasName("value"), ofClass(smartPointerClassWithValue()))));
+}
+
+ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeGetMethodCall() {
+ return cxxMemberCallExpr(callee(
+ cxxMethodDecl(parameterCountIs(0), returns(pointerType()), hasName("get"),
+ ofClass(smartPointerClassWithGet()))));
+}
+
+} // namespace clang::dataflow
diff --git a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
index 4e1819bfa166a8..6c01ae8fc2e541 100644
--- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
@@ -21,6 +21,7 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests
SignAnalysisTest.cpp
SimplifyConstraintsTest.cpp
SingleVarConstantPropagationTest.cpp
+ SmartPointerAccessorCachingTest.cpp
TestingSupport.cpp
TestingSupportTest.cpp
TransferBranchTest.cpp
diff --git a/clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp b/clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
new file mode 100644
index 00000000000000..a69d606cdb85ea
--- /dev/null
+++ b/clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
@@ -0,0 +1,194 @@
+//===- unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp ==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Testing/TestAST.h"
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+namespace clang::dataflow {
+namespace {
+
+using clang::ast_matchers::match;
+
+template <typename MatcherT>
+bool matches(llvm::StringRef Decls, llvm::StringRef TestInput,
+ MatcherT Matcher) {
+ TestAST InputAST(Decls.str() + TestInput.str());
+ return !match(Matcher, InputAST.context()).empty();
+}
+
+TEST(SmartPointerAccessorCachingTest, MatchesClassWithStarArrowGet) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct unique_ptr {
+ T* operator->() const;
+ T& operator*() const;
+ T* get() const;
+ };
+ } // namespace std
+
+ template <class T>
+ using UniquePtrAlias = std::unique_ptr<T>;
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return (*P).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P.get()->i; }",
+ isSmartPointerLikeGetMethodCall()));
+
+ EXPECT_TRUE(matches(Decls, "int target(UniquePtrAlias<S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfUnexpectedReturnTypes) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ // unique_ptr isn't really like this, but we aren't matching by name
+ template <class T, class U>
+ struct unique_ptr {
+ U* operator->() const;
+ T& operator*() const;
+ T* get() const;
+ };
+ } // namespace std
+
+ struct S { int i; };
+ struct T { int j; };
+ )cc");
+
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S, T> P) { return (*P).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S, T> P) { return P->j; }",
+ isSmartPointerLikeOperatorArrow()));
+ // The class matching arguably accidentally matches, just because the
+ // instantiation is with S, S. Hopefully doesn't happen too much in real code
+ // with such operator* and operator-> overloads.
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr<S, S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfBinaryStar) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct unique_ptr {
+ T* operator->() const;
+ T& operator*(int x) const;
+ T* get() const;
+ };
+ } // namespace std
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_FALSE(
+ matches(Decls, "int target(std::unique_ptr<S> P) { return (P * 10).i; }",
+ isSmartPointerLikeOperatorStar()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfNoConstOverloads) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct unique_ptr {
+ T* operator->();
+ T& operator*();
+ T* get();
+ };
+ } // namespace std
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return (*P).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_FALSE(
+ matches(Decls, "int target(std::unique_ptr<S> P) { return P.get()->i; }",
+ isSmartPointerLikeGetMethodCall()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfNoStarMethod) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct unique_ptr {
+ T* operator->();
+ T* get();
+ };
+ } // namespace std
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr<S> P) { return P->i; }",
+ isSmartPointerLikeGetMethodCall()));
+}
+
+TEST(SmartPointerAccessorCachingTest, MatchesWithValueAndNonConstOverloads) {
+ llvm::StringRef Decls(R"cc(
+ namespace std {
+ template <class T>
+ struct optional {
+ const T* operator->() const;
+ T* operator->();
+ const T& operator*() const;
+ T& operator*();
+ const T& value() const;
+ T& value();
+ };
+ } // namespace std
+
+ struct S { int i; };
+ )cc");
+
+ EXPECT_TRUE(matches(
+ Decls, "int target(std::optional<S> &NonConst) { return (*NonConst).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_TRUE(matches(
+ Decls, "int target(const std::optional<S> &Const) { return (*Const).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_TRUE(matches(
+ Decls, "int target(std::optional<S> &NonConst) { return NonConst->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_TRUE(matches(
+ Decls, "int target(const std::optional<S> &Const) { return Const->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_TRUE(matches(
+ Decls,
+ "int target(std::optional<S> &NonConst) { return NonConst.value().i; }",
+ isSmartPointerLikeValueMethodCall()));
+ EXPECT_TRUE(matches(
+ Decls,
+ "int target(const std::optional<S> &Const) { return Const.value().i; }",
+ isSmartPointerLikeValueMethodCall()));
+}
+
+} // namespace
+} // namespace clang::dataflow
\ No newline at end of file
|
clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
Outdated
Show resolved
Hide resolved
clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
Outdated
Show resolved
Hide resolved
|
||
namespace clang::dataflow { | ||
|
||
ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how these matchers would be used but I wonder if we end up doing a lot of duplicated work using this paradigm. Alternatively, we could just match the smart pointer class once in one matcher, and bound all points of interests to certain names. The user can later on use these names to check if there was a match for the operators we are interested in or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, it wasn't clear how these are used! I uploaded a draft of the user to https://github.com/llvm/llvm-project/pull/120249/files#diff-068c9724d855a9686b7ca6573bf3e966d856f2e4200d0eba45db0738e9c9e7afR1038 in case that helps (source examples in the test there).
I'm not too sure how to match the smart pointer class once and reuse (not as familiar with the matcher binding yet!) across different call ASTs, but I can imagine doing a pre-pass that computed smart pointer classes and then use that in the matcher.
So far, I hadn't seen the naive approach be slow in profiles, so I hadn't tried optimizing. If the matchers short circuit, then maybe it doesn't try to do the smart pointer class checks often? For example, if it has to match the 0 param check, and match the name first? With the names being disjoint, on an example call ptr.get().x.value();
it would match the hasName("get")
and then do the smart pointer class match on the type of ptr
but it wouldn't have passed the hasOverloadedOperatorName("->")
check and wouldn't redundantly try to check the class for that case (or * or value)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case this hasn't showed up in profiles yet, I am OK with the current approach. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- will keep the idea of matching once and reusing the result in mind if performance becomes an issue later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure our choice to use matchers in our analyses in general is costly, but I don't think that this particular use of matchers will likely have much impact. That said, jvoung -- do any of our performance benchmarks exercise these cases? I would expect that code with heavy use of smart pointers could see some impact from repeatedly computing smartPointerClassWithGetOrValue
. But, I agree that we should only optimize if we have evidence of it being a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes in the profiles I am seeing more time in matchers generally and not much in the .*martPointer.*
functions.
I tried running before/after on ~20 internal files (some with ~5000 lines of code + 100 direct #include
s). The geomean diff is 0.01%, though for two files the time increased by 20% (~1s wall time). For most files there is a minor decrease (perhaps caching reduces the work later?) and for one file the checker time decreased by 15% (~0.4s wall time). In the profile of one of the 20% increases, it seems the solver time increased (maybe the formulas get more complicated in some way). The smartPointerClassWithGetOrValue
function is only called about 300 times in that example.
I also tried removing the check filter for HasOptionalCallDescendant
here
llvm-project/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
Line 36 in b5d0278
hasBody(HasOptionalCallDescendant)), |
✅ With the latest revision this PR passed the C/C++ code formatter. |
…essor Part 2 (and final part) following llvm#120102 Allows users to do things like: ``` if (o->x.has_value()) { ((*o).x).value(); } ``` where the `->` and `*` are operator overload calls. A user could instead extract the nested optional into a local variable once instead of doing two accessor calls back to back, but currently they are unsure why the code is flagged.
/// for `std::optional`, we assume the (Matcher, TransferFunction) case | ||
/// with custom handling is ordered early so that these generic cases | ||
/// do not trigger. | ||
ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use the alias ast_matchers::StatementMatcher
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks! done
|
||
namespace clang::dataflow { | ||
|
||
ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure our choice to use matchers in our analyses in general is costly, but I don't think that this particular use of matchers will likely have much impact. That said, jvoung -- do any of our performance benchmarks exercise these cases? I would expect that code with heavy use of smart pointers could see some impact from repeatedly computing smartPointerClassWithGetOrValue
. But, I agree that we should only optimize if we have evidence of it being a problem.
// there should at least be a const overload as well. | ||
if (!MD->isConst() || MD->getNumParams() != 0) | ||
continue; | ||
if (MD->getOverloadedOperator() == OO_Star && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider a switch on MD->getOverloadedOperator()
. As is, for OO_Star and OO_Arrow, when the second clause fails, you'll fall through to the "else" case, but really want to just continue, so the switch is a little more "correct" from that perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! done
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/10750 Here is the relevant piece of the build log for the reference
|
FYI:
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/5044 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/8053 Here is the relevant piece of the build log for the reference
|
Ack sorry about that. |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/6815 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/7814 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/6514 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/3965 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/1777 Here is the relevant piece of the build log for the reference
|
…essor (#120249) Part 2 (and final part) following #120102 Allows users to do things like: ``` if (o->x.has_value()) { ((*o).x).value(); } ``` where the `->` and `*` are operator overload calls. A user could instead extract the nested optional into a local variable once instead of doing two accessor calls back to back, but currently they are unsure why the code is flagged.
…Wimplicit-fallthrough (#120739) Missed when changing code in llvm/llvm-project#120102
…ptional accessor (#120249) Part 2 (and final part) following llvm/llvm-project#120102 Allows users to do things like: ``` if (o->x.has_value()) { ((*o).x).value(); } ``` where the `->` and `*` are operator overload calls. A user could instead extract the nested optional into a local variable once instead of doing two accessor calls back to back, but currently they are unsure why the code is flagged.
This is part 1 of caching for smart pointer accessors, building on top
of the CachedConstAccessorsLattice, which caches "normal" accessors.
Smart pointer accessors are a bit different in that they may:
returning slightly different types like
&
vs*
). Within a"checked" sequence users may mix uses of the different aliases and the
check should apply to any of the spellings.
the non-const doesn't actually modify the container
Part 2 will follow and add transfer functions utilities. It will also
add a user UncheckedOptionalAccessModel. We'd seen false positives when
nesting StatusOr<optional> and optional<StatusOr>, etc. which this
can help address.