Skip to content
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

[clang][dataflow] Disambiguate a ref to "internal" in CachedConstAccessorsLattice #113601

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

jvoung
Copy link
Contributor

@jvoung jvoung commented Oct 24, 2024

Disambiguate to fix a build error (e.g., on windows with clang-cl)

…ssorsLattice

Disambiguate to fix a build error (e.g., on windows with clang-cl)
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:analysis labels Oct 24, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 24, 2024

@llvm/pr-subscribers-clang-analysis

Author: Jan Voung (jvoung)

Changes

Disambiguate to fix a build error (e.g., on windows with clang-cl)


Full diff: https://github.com/llvm/llvm-project/pull/113601.diff

1 Files Affected:

  • (modified) clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h (+4-3)
diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 3402d105746e88..48c5287367739a 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -154,11 +154,12 @@ LatticeEffect CachedConstAccessorsLattice<Base>::join(
   // are non-identical but equivalent. This is likely to be sufficient in
   // practice, and it reduces implementation complexity considerably.
 
-  ConstMethodReturnValues = internal::joinConstMethodMap<Value>(
-      ConstMethodReturnValues, Other.ConstMethodReturnValues, Effect);
+  ConstMethodReturnValues =
+      clang::dataflow::internal::joinConstMethodMap<dataflow::Value>(
+          ConstMethodReturnValues, Other.ConstMethodReturnValues, Effect);
 
   ConstMethodReturnStorageLocations =
-      internal::joinConstMethodMap<StorageLocation>(
+      clang::dataflow::internal::joinConstMethodMap<dataflow::StorageLocation>(
           ConstMethodReturnStorageLocations,
           Other.ConstMethodReturnStorageLocations, Effect);
 

@llvmbot
Copy link
Member

llvmbot commented Oct 24, 2024

@llvm/pr-subscribers-clang

Author: Jan Voung (jvoung)

Changes

Disambiguate to fix a build error (e.g., on windows with clang-cl)


Full diff: https://github.com/llvm/llvm-project/pull/113601.diff

1 Files Affected:

  • (modified) clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h (+4-3)
diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 3402d105746e88..48c5287367739a 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -154,11 +154,12 @@ LatticeEffect CachedConstAccessorsLattice<Base>::join(
   // are non-identical but equivalent. This is likely to be sufficient in
   // practice, and it reduces implementation complexity considerably.
 
-  ConstMethodReturnValues = internal::joinConstMethodMap<Value>(
-      ConstMethodReturnValues, Other.ConstMethodReturnValues, Effect);
+  ConstMethodReturnValues =
+      clang::dataflow::internal::joinConstMethodMap<dataflow::Value>(
+          ConstMethodReturnValues, Other.ConstMethodReturnValues, Effect);
 
   ConstMethodReturnStorageLocations =
-      internal::joinConstMethodMap<StorageLocation>(
+      clang::dataflow::internal::joinConstMethodMap<dataflow::StorageLocation>(
           ConstMethodReturnStorageLocations,
           Other.ConstMethodReturnStorageLocations, Effect);
 

@jvoung jvoung merged commit 8aa69a0 into llvm:main Oct 24, 2024
12 checks passed
@nico
Copy link
Contributor

nico commented Oct 24, 2024

I think this'll fix the build, thanks!

However, the root cause is probably that the AST_MATCHER_P_OVERLOAD instantiation in clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp is in an unnamed namespace that's inside namespace clang::dataflow, right? Should we move that to just an unnamed namespace at the top of that file that isn't in clang::dataflow?

@frobtech frobtech mentioned this pull request Oct 25, 2024
@jvoung
Copy link
Contributor Author

jvoung commented Oct 25, 2024

AST_MATCHER_P_OVERLOAD

Ah, missed the part where the macro is generating the internal namespaces. I don't think we want to move to the top of the file exactly (the instantiation refers to some functions defined right above, so would need fwd decls?), but we can change the namespacing inline } ... {.

There seem to be other recommendations in the header


like putting in clang::ast_matchers::unnamed... (though I'm not sure if they mean exactly clang::ast_matchers.

Let me try and check with the team.

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…ssorsLattice (llvm#113601)

Disambiguate to fix a build error (e.g., on windows with clang-cl)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants