[clang] [llvm] [clang] Add/enhance documentation for some important classes. (PR #109795)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall updated 
https://github.com/llvm/llvm-project/pull/109795

>From 8dd7d0afc65526f152a02cbd5772ba9882cc2614 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Tue, 24 Sep 2024 15:02:36 +0200
Subject: [PATCH 1/3] [clang] Add/enhance documentation for some important
 classes.

---
 clang/include/clang/AST/DeclBase.h| 52 +++--
 clang/include/clang/AST/Stmt.h| 18 -
 clang/include/clang/AST/Type.h| 40 +-
 clang/include/clang/AST/TypeLoc.h | 78 ++-
 clang/include/clang/Basic/SourceManager.h | 56 ++---
 .../llvm/Support/FileSystem/UniqueID.h| 11 +--
 llvm/include/llvm/Support/VirtualFileSystem.h | 19 -
 7 files changed, 209 insertions(+), 65 deletions(-)

diff --git a/clang/include/clang/AST/DeclBase.h 
b/clang/include/clang/AST/DeclBase.h
index ee662ed73d7e0e..8b76cd43c1d62a 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -76,13 +76,26 @@ enum AvailabilityResult {
   AR_Unavailable
 };
 
-/// Decl - This represents one declaration (or definition), e.g. a variable,
-/// typedef, function, struct, etc.
+/// A Decl describes a declaration (or definition) of a variable, function, 
etc.
+/// This is the base class for a hierarchy of Decls: VarDecl, FunctionDecl...
 ///
-/// Note: There are objects tacked on before the *beginning* of Decl
-/// (and its subclasses) in its Decl::operator new(). Proper alignment
-/// of all subclasses (not requiring more than the alignment of Decl) is
-/// asserted in DeclBase.cpp.
+/// The declarations form a tree rooted at the TranslationUnitDecl.
+/// The non-leaf nodes of this tree are DeclContexts (as well as being Decls).
+///
+/// Decls are also the AST's representation of the things being declared.
+/// So a VarDecl* may refer either to a specific declaration of a variable, or
+/// to the variable itself - pointing at an arbitrary declaration of it.
+/// (Declarations of the same variable are linked in a "redecl chain", and
+/// the first entry is the canonical representative when one is needed).
+///
+/// Some entities have zero declarations in code, like implicit constructors.
+/// For these, a Decl is synthesized and marked "implicit". Lexical information
+/// like SourceLocations may not be meaningful for such Decls.
+///
+/// Like other AST nodes, Decls are allocated within an ASTContext, using
+/// factory functions like FooDecl::Create(Ctx, ...).
+/// This may allocate leading data before the object (see Decl::operator new)
+/// and trailing data after it (see e.g. CXXConstructorDecl::Create).
 class alignas(8) Decl {
 public:
   /// Lists the kind of concrete classes of Decl.
@@ -1416,23 +1429,18 @@ enum class OMPDeclareReductionInitKind;
 enum class ObjCImplementationControl;
 enum class LinkageSpecLanguageIDs;
 
-/// DeclContext - This is used only as base class of specific decl types that
-/// can act as declaration contexts. These decls are (only the top classes
-/// that directly derive from DeclContext are mentioned, not their subclasses):
+/// A declaration context is a Decl that can contain other declarations.
+///
+/// Declarations form a tree rooted at the single TranslationUnitDecl, and
+/// the non-leaf nodes are DeclContexts.
+///
+/// Contexts are important for name lookup, which usually involves querying
+/// several contexts in sequence. (However some local lookup scopes such as
+/// CompoundStmt are not DeclContexts - see clang::Scope).
 ///
-///   TranslationUnitDecl
-///   ExternCContext
-///   NamespaceDecl
-///   TagDecl
-///   OMPDeclareReductionDecl
-///   OMPDeclareMapperDecl
-///   FunctionDecl
-///   ObjCMethodDecl
-///   ObjCContainerDecl
-///   LinkageSpecDecl
-///   ExportDecl
-///   BlockDecl
-///   CapturedDecl
+/// DeclContext is a second base class for the relevant Decl subclasses,
+/// e.g. FunctionDecl inherits from both DeclaratorDecl and DeclContext.
+/// It is safe to cast(&DC), as DeclContexts are always Decls.
 class DeclContext {
   /// For makeDeclVisibleInContextImpl
   friend class ASTDeclReader;
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 7aed83e9c68bb7..07133c245f0027 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -79,8 +79,24 @@ enum class StringLiteralKind;
 // AST classes for statements.
 
//===--===//
 
-/// Stmt - This represents one statement.
+/// A statement or expression in the program.
 ///
+/// This is the base for the hierarchy of statements (ForStmt, ReturnStmt...)
+/// as well as expressions (Expr, CastExpr, IntegerLiteral...).
+/// Classing expressions as Stmt allows them to appear as statements without
+/// needing an extra "expression-statement" node.
+///
+/// Statements can have children and so form trees. e.g. `while (i>0) i--;`
+///
+///   WhileStmt
+///   |-BinaryOperator >
+///  

[clang] [llvm] [clang] Add/enhance documentation for some important classes. (PR #109795)

[Sam McCall via cfe-commits]

@@ -79,8 +79,24 @@ enum class StringLiteralKind;
 // AST classes for statements.
 
//===--===//
 
-/// Stmt - This represents one statement.
+/// A statement or expression in the program.
 ///
+/// This is the base for the hierarchy of statements (ForStmt, ReturnStmt...)
+/// as well as expressions (Expr, CastExpr, IntegerLiteral...).
+/// Classing expressions as Stmt allows them to appear as statements without
+/// needing an extra "expression-statement" node.
+///
+/// Statements can have children and so form trees. e.g. `while (i>0) i--;`
+///
+///   WhileStmt
+///   |-BinaryOperator >
+///   | |-DeclRefExpr i
+///   | `-IntegerLiteral 0
+///   `-UnaryOperator --
+///   DeclRefExpr i
+///

sam-mccall wrote:

Thanks, this was indeed being mangled.

I've instead indented by two more spaces. Doxygen understands this part of 
markdown, and I think it keeps the source file more readable.


https://github.com/llvm/llvm-project/pull/109795
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang] Add/enhance documentation for some important classes. (PR #109795)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall updated 
https://github.com/llvm/llvm-project/pull/109795

>From 8dd7d0afc65526f152a02cbd5772ba9882cc2614 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Tue, 24 Sep 2024 15:02:36 +0200
Subject: [PATCH 1/2] [clang] Add/enhance documentation for some important
 classes.

---
 clang/include/clang/AST/DeclBase.h| 52 +++--
 clang/include/clang/AST/Stmt.h| 18 -
 clang/include/clang/AST/Type.h| 40 +-
 clang/include/clang/AST/TypeLoc.h | 78 ++-
 clang/include/clang/Basic/SourceManager.h | 56 ++---
 .../llvm/Support/FileSystem/UniqueID.h| 11 +--
 llvm/include/llvm/Support/VirtualFileSystem.h | 19 -
 7 files changed, 209 insertions(+), 65 deletions(-)

diff --git a/clang/include/clang/AST/DeclBase.h 
b/clang/include/clang/AST/DeclBase.h
index ee662ed73d7e0e..8b76cd43c1d62a 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -76,13 +76,26 @@ enum AvailabilityResult {
   AR_Unavailable
 };
 
-/// Decl - This represents one declaration (or definition), e.g. a variable,
-/// typedef, function, struct, etc.
+/// A Decl describes a declaration (or definition) of a variable, function, 
etc.
+/// This is the base class for a hierarchy of Decls: VarDecl, FunctionDecl...
 ///
-/// Note: There are objects tacked on before the *beginning* of Decl
-/// (and its subclasses) in its Decl::operator new(). Proper alignment
-/// of all subclasses (not requiring more than the alignment of Decl) is
-/// asserted in DeclBase.cpp.
+/// The declarations form a tree rooted at the TranslationUnitDecl.
+/// The non-leaf nodes of this tree are DeclContexts (as well as being Decls).
+///
+/// Decls are also the AST's representation of the things being declared.
+/// So a VarDecl* may refer either to a specific declaration of a variable, or
+/// to the variable itself - pointing at an arbitrary declaration of it.
+/// (Declarations of the same variable are linked in a "redecl chain", and
+/// the first entry is the canonical representative when one is needed).
+///
+/// Some entities have zero declarations in code, like implicit constructors.
+/// For these, a Decl is synthesized and marked "implicit". Lexical information
+/// like SourceLocations may not be meaningful for such Decls.
+///
+/// Like other AST nodes, Decls are allocated within an ASTContext, using
+/// factory functions like FooDecl::Create(Ctx, ...).
+/// This may allocate leading data before the object (see Decl::operator new)
+/// and trailing data after it (see e.g. CXXConstructorDecl::Create).
 class alignas(8) Decl {
 public:
   /// Lists the kind of concrete classes of Decl.
@@ -1416,23 +1429,18 @@ enum class OMPDeclareReductionInitKind;
 enum class ObjCImplementationControl;
 enum class LinkageSpecLanguageIDs;
 
-/// DeclContext - This is used only as base class of specific decl types that
-/// can act as declaration contexts. These decls are (only the top classes
-/// that directly derive from DeclContext are mentioned, not their subclasses):
+/// A declaration context is a Decl that can contain other declarations.
+///
+/// Declarations form a tree rooted at the single TranslationUnitDecl, and
+/// the non-leaf nodes are DeclContexts.
+///
+/// Contexts are important for name lookup, which usually involves querying
+/// several contexts in sequence. (However some local lookup scopes such as
+/// CompoundStmt are not DeclContexts - see clang::Scope).
 ///
-///   TranslationUnitDecl
-///   ExternCContext
-///   NamespaceDecl
-///   TagDecl
-///   OMPDeclareReductionDecl
-///   OMPDeclareMapperDecl
-///   FunctionDecl
-///   ObjCMethodDecl
-///   ObjCContainerDecl
-///   LinkageSpecDecl
-///   ExportDecl
-///   BlockDecl
-///   CapturedDecl
+/// DeclContext is a second base class for the relevant Decl subclasses,
+/// e.g. FunctionDecl inherits from both DeclaratorDecl and DeclContext.
+/// It is safe to cast(&DC), as DeclContexts are always Decls.
 class DeclContext {
   /// For makeDeclVisibleInContextImpl
   friend class ASTDeclReader;
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 7aed83e9c68bb7..07133c245f0027 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -79,8 +79,24 @@ enum class StringLiteralKind;
 // AST classes for statements.
 
//===--===//
 
-/// Stmt - This represents one statement.
+/// A statement or expression in the program.
 ///
+/// This is the base for the hierarchy of statements (ForStmt, ReturnStmt...)
+/// as well as expressions (Expr, CastExpr, IntegerLiteral...).
+/// Classing expressions as Stmt allows them to appear as statements without
+/// needing an extra "expression-statement" node.
+///
+/// Statements can have children and so form trees. e.g. `while (i>0) i--;`
+///
+///   WhileStmt
+///   |-BinaryOperator >
+///  

[clang] [llvm] [clang] Add/enhance documentation for some important classes. (PR #109795)

[Sam McCall via cfe-commits]

sam-mccall wrote:

There are some classes here where I think the concepts are tricky and 
under-explained (SourceManager, TypeLoc) or just important enough that it's 
worth spending a few more words in general.
(I'm sure there are a bunch more, but I hit my timebox)

If you think anything is too wordy/confusing or I'm removing anything 
important, please do say so!

Let me know if it's better to split these into smaller patches.

https://github.com/llvm/llvm-project/pull/109795
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang] Add/enhance documentation for some important classes. (PR #109795)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall created 
https://github.com/llvm/llvm-project/pull/109795

None

>From 8dd7d0afc65526f152a02cbd5772ba9882cc2614 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Tue, 24 Sep 2024 15:02:36 +0200
Subject: [PATCH] [clang] Add/enhance documentation for some important classes.

---
 clang/include/clang/AST/DeclBase.h| 52 +++--
 clang/include/clang/AST/Stmt.h| 18 -
 clang/include/clang/AST/Type.h| 40 +-
 clang/include/clang/AST/TypeLoc.h | 78 ++-
 clang/include/clang/Basic/SourceManager.h | 56 ++---
 .../llvm/Support/FileSystem/UniqueID.h| 11 +--
 llvm/include/llvm/Support/VirtualFileSystem.h | 19 -
 7 files changed, 209 insertions(+), 65 deletions(-)

diff --git a/clang/include/clang/AST/DeclBase.h 
b/clang/include/clang/AST/DeclBase.h
index ee662ed73d7e0e..8b76cd43c1d62a 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -76,13 +76,26 @@ enum AvailabilityResult {
   AR_Unavailable
 };
 
-/// Decl - This represents one declaration (or definition), e.g. a variable,
-/// typedef, function, struct, etc.
+/// A Decl describes a declaration (or definition) of a variable, function, 
etc.
+/// This is the base class for a hierarchy of Decls: VarDecl, FunctionDecl...
 ///
-/// Note: There are objects tacked on before the *beginning* of Decl
-/// (and its subclasses) in its Decl::operator new(). Proper alignment
-/// of all subclasses (not requiring more than the alignment of Decl) is
-/// asserted in DeclBase.cpp.
+/// The declarations form a tree rooted at the TranslationUnitDecl.
+/// The non-leaf nodes of this tree are DeclContexts (as well as being Decls).
+///
+/// Decls are also the AST's representation of the things being declared.
+/// So a VarDecl* may refer either to a specific declaration of a variable, or
+/// to the variable itself - pointing at an arbitrary declaration of it.
+/// (Declarations of the same variable are linked in a "redecl chain", and
+/// the first entry is the canonical representative when one is needed).
+///
+/// Some entities have zero declarations in code, like implicit constructors.
+/// For these, a Decl is synthesized and marked "implicit". Lexical information
+/// like SourceLocations may not be meaningful for such Decls.
+///
+/// Like other AST nodes, Decls are allocated within an ASTContext, using
+/// factory functions like FooDecl::Create(Ctx, ...).
+/// This may allocate leading data before the object (see Decl::operator new)
+/// and trailing data after it (see e.g. CXXConstructorDecl::Create).
 class alignas(8) Decl {
 public:
   /// Lists the kind of concrete classes of Decl.
@@ -1416,23 +1429,18 @@ enum class OMPDeclareReductionInitKind;
 enum class ObjCImplementationControl;
 enum class LinkageSpecLanguageIDs;
 
-/// DeclContext - This is used only as base class of specific decl types that
-/// can act as declaration contexts. These decls are (only the top classes
-/// that directly derive from DeclContext are mentioned, not their subclasses):
+/// A declaration context is a Decl that can contain other declarations.
+///
+/// Declarations form a tree rooted at the single TranslationUnitDecl, and
+/// the non-leaf nodes are DeclContexts.
+///
+/// Contexts are important for name lookup, which usually involves querying
+/// several contexts in sequence. (However some local lookup scopes such as
+/// CompoundStmt are not DeclContexts - see clang::Scope).
 ///
-///   TranslationUnitDecl
-///   ExternCContext
-///   NamespaceDecl
-///   TagDecl
-///   OMPDeclareReductionDecl
-///   OMPDeclareMapperDecl
-///   FunctionDecl
-///   ObjCMethodDecl
-///   ObjCContainerDecl
-///   LinkageSpecDecl
-///   ExportDecl
-///   BlockDecl
-///   CapturedDecl
+/// DeclContext is a second base class for the relevant Decl subclasses,
+/// e.g. FunctionDecl inherits from both DeclaratorDecl and DeclContext.
+/// It is safe to cast(&DC), as DeclContexts are always Decls.
 class DeclContext {
   /// For makeDeclVisibleInContextImpl
   friend class ASTDeclReader;
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 7aed83e9c68bb7..07133c245f0027 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -79,8 +79,24 @@ enum class StringLiteralKind;
 // AST classes for statements.
 
//===--===//
 
-/// Stmt - This represents one statement.
+/// A statement or expression in the program.
 ///
+/// This is the base for the hierarchy of statements (ForStmt, ReturnStmt...)
+/// as well as expressions (Expr, CastExpr, IntegerLiteral...).
+/// Classing expressions as Stmt allows them to appear as statements without
+/// needing an extra "expression-statement" node.
+///
+/// Statements can have children and so form trees. e.g. `while (i>0) i--;`
+///
+///   WhileStmt
+///   |-BinaryOperator >
+/// 

[clang-tools-extra] [include-cleaner] don't consider the associated header unused (PR #67228)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall closed 
https://github.com/llvm/llvm-project/pull/67228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [include-cleaner] don't consider the associated header unused (PR #67228)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall updated 
https://github.com/llvm/llvm-project/pull/67228

>From a736ebb34a31fa0fd1c21a4f03c88d63856fb998 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Sat, 23 Sep 2023 10:44:30 +0200
Subject: [PATCH] [include-cleaner] don't consider the associated header unused

Loosely, the "associated header" of `foo.cpp` is `foo.h`.
It should be included, many styles include it first.

So far we haven't special cased it in any way, and require this include
to be justified. e.g. if foo.cpp defines a function declared in foo.h,
then the #include is allowed to check these declarations match.

However this doesn't really align with what users want:
- people reasonably want to include the associated header for the
  side-effect of validating that it compiles.
  In the degenerate case, `lib.cpp`is just `#include "lib.h"` (see bug)
- That `void foo(){}` IWYU-uses `void foo();` is a bit artificial, and
  most users won't internalize this. Instead they'll stick with the
  simpler model "include the header that defines your API".
  In the rare cases where these give different answers[1], our current
  behavior is a puzzling special case from the user POV.
  It is more user-friendly to accept both models.
- even where this diagnostic is a true positive (defs don't match header
  decls) the diagnostic does not communicate this usefully.

Fixes https://github.com/llvm/llvm-project/issues/67140

[1] Example of an associated header that's not IWYU-used:
```
// http.h
inline URL buildHttpURL(string host, int port, string path) {
  return "http://"; + host + ":" + port + "/" + path;
}
// http.cpp
class HTTPURLHandler : URLHandler { ... };
REGISTER_URL_HANDLER("http", HTTPURLHandler);
```
---
 .../include-cleaner/lib/Record.cpp| 36 ++-
 .../include-cleaner/unittests/RecordTest.cpp  | 59 ++-
 2 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp 
b/clang-tools-extra/include-cleaner/lib/Record.cpp
index 78a4df6cc40ea..6b5be956ec108 100644
--- a/clang-tools-extra/include-cleaner/lib/Record.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -34,6 +34,7 @@
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem/UniqueID.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
@@ -180,7 +181,9 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, 
public CommentHandler {
   RecordPragma(const Preprocessor &P, PragmaIncludes *Out)
   : SM(P.getSourceManager()), HeaderInfo(P.getHeaderSearchInfo()), 
Out(Out),
 Arena(std::make_shared()),
-UniqueStrings(*Arena) {}
+UniqueStrings(*Arena),
+MainFileStem(llvm::sys::path::stem(
+SM.getNonBuiltinFilenameForID(SM.getMainFileID()).value_or(""))) {}
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -228,8 +231,9 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, 
public CommentHandler {
   }
 if (!IncludedHeader && File)
   IncludedHeader = *File;
-checkForExport(HashFID, HashLine, std::move(IncludedHeader), File);
+checkForExport(HashFID, HashLine, IncludedHeader, File);
 checkForKeep(HashLine, File);
+checkForDeducedAssociated(IncludedHeader);
   }
 
   void checkForExport(FileID IncludingFile, int HashLine,
@@ -269,6 +273,27 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, 
public CommentHandler {
   KeepStack.pop_back(); // Pop immediately for single-line keep pragma.
   }
 
+  // Consider marking H as the "associated header" of the main file.
+  //
+  // Our heuristic:
+  // - it must be the first #include in the main file
+  // - it must have the same name stem as the main file (foo.h and foo.cpp)
+  // (IWYU pragma: associated is also supported, just not by this function).
+  //
+  // We consider the associated header as if it had a keep pragma.
+  // (Unlike IWYU, we don't treat #includes inside the associated header as if
+  // they were written in the main file.)
+  void checkForDeducedAssociated(std::optional H) {
+namespace path = llvm::sys::path;
+if (!InMainFile || SeenAssociatedCandidate)
+  return;
+SeenAssociatedCandidate = true; // Only the first #include is our 
candidate.
+if (!H || H->kind() != Header::Physical)
+  return;
+if (path::stem(H->physical().getName(), path::Style::posix) == 
MainFileStem)
+  Out->ShouldKeep.insert(H->physical().getUniqueID());
+  }
+
   bool HandleComment(Preprocessor &PP, SourceRange Range) override {
 auto &SM = PP.getSourceManager();
 auto Pragma =
@@ -280,7 +305,9 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, 
public CommentHandler {
 int CommentLine = SM.getLineNumber(CommentFID, CommentOffset);
 
 if (InMainFile) {
-  if (Pragma->starts_with("keep")) {
+  if (Pragma->starts_with

[clang-tools-extra] [include-cleaner] don't consider the associated header unused (PR #67228)

[Sam McCall via cfe-commits]

sam-mccall wrote:

Apologies & thanks all for the pings.
I unfortunately won't be active on llvm-project for a while (other work 
commitments), but I can get this landed.

https://github.com/llvm/llvm-project/pull/67228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [include-cleaner] don't consider the associated header unused (PR #67228)

[Sam McCall via cfe-commits]

@@ -379,6 +379,54 @@ TEST_F(PragmaIncludeTest, IWYUKeep) {
   EXPECT_TRUE(PI.shouldKeep(FM.getFile("std/set").get()));
 }
 
+TEST_F(PragmaIncludeTest, AssociatedHeader) {
+  createEmptyFiles({"foo/main.h", "bar/main.h", "bar/other.h", "std/vector"});
+  auto IsKeep = [&](llvm::StringRef Name, TestAST &AST) {
+return PI.shouldKeep(AST.fileManager().getFile(Name).get());
+  };
+
+  Inputs.FileName = "main.cc";
+  Inputs.ExtraArgs.push_back("-isystemstd");
+  {
+Inputs.Code = R"cpp(
+  #include "foo/main.h"
+  #include "bar/main.h"
+)cpp";
+auto AST = build();
+EXPECT_TRUE(IsKeep("foo/main.h", AST));
+EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
+  }
+
+  {
+Inputs.Code = R"cpp(
+  #include "bar/other.h"
+  #include "bar/main.h"
+)cpp";
+auto AST = build();
+EXPECT_FALSE(IsKeep("bar/other.h", AST));
+EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
+  }
+
+  {
+Inputs.Code = R"cpp(
+  #include "foo/main.h"
+  #include "bar/other.h" // IWYU pragma: associated

sam-mccall wrote:

Sure. Combined this with a test that associated on stdlib headers is honored.

(As a bonus, I found that we were reusing PragmaIncludes across testcases, 
fixed that too!)

https://github.com/llvm/llvm-project/pull/67228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [include-cleaner] don't consider the associated header unused (PR #67228)

[Sam McCall via cfe-commits]

@@ -379,6 +379,54 @@ TEST_F(PragmaIncludeTest, IWYUKeep) {
   EXPECT_TRUE(PI.shouldKeep(FM.getFile("std/set").get()));
 }
 
+TEST_F(PragmaIncludeTest, AssociatedHeader) {
+  createEmptyFiles({"foo/main.h", "bar/main.h", "bar/other.h", "std/vector"});
+  auto IsKeep = [&](llvm::StringRef Name, TestAST &AST) {
+return PI.shouldKeep(AST.fileManager().getFile(Name).get());
+  };
+
+  Inputs.FileName = "main.cc";
+  Inputs.ExtraArgs.push_back("-isystemstd");
+  {
+Inputs.Code = R"cpp(
+  #include "foo/main.h"
+  #include "bar/main.h"
+)cpp";
+auto AST = build();
+EXPECT_TRUE(IsKeep("foo/main.h", AST));
+EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
+  }
+
+  {
+Inputs.Code = R"cpp(
+  #include "bar/other.h"
+  #include "bar/main.h"
+)cpp";
+auto AST = build();
+EXPECT_FALSE(IsKeep("bar/other.h", AST));
+EXPECT_FALSE(IsKeep("bar/main.h", AST)) << "not first include";
+  }
+
+  {
+Inputs.Code = R"cpp(
+  #include "foo/main.h"
+  #include "bar/other.h" // IWYU pragma: associated
+)cpp";
+auto AST = build();
+EXPECT_TRUE(IsKeep("foo/main.h", AST));
+EXPECT_TRUE(IsKeep("bar/other.h", AST));
+  }
+
+  Inputs.FileName = "vector.cc";
+  {
+Inputs.Code = R"cpp(
+  #include 
+)cpp";
+auto AST = build();
+EXPECT_FALSE(IsKeep("std/vector", AST)) << "stdlib is never associated";

sam-mccall wrote:

sure, s/never/not/.

https://github.com/llvm/llvm-project/pull/67228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [include-cleaner] don't consider the associated header unused (PR #67228)

[Sam McCall via cfe-commits]

@@ -227,6 +230,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks, 
public CommentHandler {
   IncludedHeader = *File;
 checkForExport(HashFID, HashLine, std::move(IncludedHeader), File);

sam-mccall wrote:

Nice catch, thanks!

https://github.com/llvm/llvm-project/pull/67228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

The code looks good for the changes you want to make.

Personally I'd keep the existing pruning behavior for all duplicated nodes, and 
use cosmetic/navigation changes to clarify. But I think you probably know 
better than me what's useful, and at this point it isn't my call either way :-)


https://github.com/llvm/llvm-project/pull/66887
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] Fix handling of cyclical data structures in HTMLLogger. (PR #66887)

[Sam McCall via cfe-commits]

@@ -88,10 +88,12 @@ class ModelDumper {
 
   void dump(Value &V) {
 JOS.attribute("value_id", llvm::to_string(&V));
-if (!Visited.insert(&V).second)
-  return;
-
 JOS.attribute("kind", debugString(V.getKind()));
+if (!Visited.insert(&V).second) {
+  JOS.attribute("[in_cycle]", " ");
+  return;
+}
+auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&V); });

sam-mccall wrote:

(Less invasive ideas that might be useful either way: "undefined" is a bug 
which can be fixed. Duplicate nodes could/should also be made a different color)

I think showing a subtree twice is a pretty serious misrepresentation of the 
data, probably more so than pruning children from a duplicated node. There's no 
easy + perfect way to show a DAG in a tree-browser. It'd be possible to make 
this more explicit (e.g. have a "duplicate node" box contain a link to an 
anchor on the original node). But it's complexity, and if you *don't* care 
about the DAG structure then it's still not ideal.

> I assume your concern is that we could have data structures with lots and 
> lots of repeated values, and this would bloat the JSON? Do we actually know 
> that this is a problem though?

Yes, I believe I saw this. I don't remember the details though, and it might 
have involved the old BoolValue subclasses that bloated the tree.

https://github.com/llvm/llvm-project/pull/66887
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [llvm] [clang-query] Remove support for srcloc output (PR #92442)

[Sam McCall via cfe-commits]

sam-mccall wrote:

Just wanted to say thanks - I didn't ever entirely understand how this was 
used, but the build speed/complexity has been somewhat painful. Removing 
features is thankless and sometimes risky, so I was expecting we'd live with 
that speedbump forever.

https://github.com/llvm/llvm-project/pull/92442
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Enable C++17 relaxed template template argument matching by default (PR #89807)

[Sam McCall via cfe-commits]

sam-mccall wrote:

The immediate deprecation causes a few issues:

- mechanical: we build with `-Wall -Werror -Wno-deprecated-declarations 
-Wno-deprecated-other-stuff` in part to catch driver misuse and fix it early. 
However this warning is not actionable, so now we need `-Wno-deprecated` which 
has undesirable effects and is also just changing unreasonably many things at 
once.
- mixed messages: there was a lot of confusion that this commit made the flag 
both required and deprecated. This isn't usual practice.
- scary with unclear expectations: deprecation warning means we need to migrate 
now, but we can't (not our code, authors probably won't take this seriously 
until clang-19). So our toolchain could break any day now?
- compatibility: there's no non-deprecated argv that results in the same 
behavior before and after this commit. What is a build system supposed to do if 
it can't version-lock clang? (Google actually *does* version-lock clang, this 
is only tangentially relevant to us because of clang-tidy, clangd etc. More 
relevant to others)
 
I think a good alternative would be: fix behavior + change default now in 
clang-19 cycle, deprecate `-fno-relaxed` flag in clang-20 cycle, remove 
`-fno-relaxed` and deprecate `-frelaxed` in clang-21 cycle. I believe this is 
more in line with what we've done in the past. Removing `-fno-relaxed` support 
is the real win here, and as this breaks deployed code doing that before 
clang-21 is probably unrealistic anyway.

---

(I forgot to mention earlier - a consequence of the crashes is that clang-tidy 
pipelines, clangd etc started crashing, which was disruptive to a lot of 
people. For clang per se it's not terribly severe to crash after emitting 
diagnostics, but other tools need to be able to consume real-world code and 
keep running, whether it's valid or not. In that sense this was more severe 
than a usual crash-on-invalid)

https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Enable C++17 relaxed template template argument matching by default (PR #89807)

[Sam McCall via cfe-commits]

sam-mccall wrote:

All makes sense to me. 

I'd point out that the only revert I was asking for was asking for was of the 
deprecation of the flag to restore the old behavior, and *optionally* the 
default flip. 

The combination of {prominent oss library, accepted by clang-18 and gcc, now 
ICE, flag to fix is deprecated} doesn't seem tenable even if the code is 
invalid, so I don't think that resolving it hinged on reproducer quality.

Others requested a broader revert. I suspect this is fix-forward being bad for 
tree stability, and revert + partial-reapply being better in this respect. 
Which... well, tradeoffs.

In any case I think we lost some nuance here, I'll try to be more helpful in 
future!

https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Enable C++17 relaxed template template argument matching by default (PR #89807)

[Sam McCall via cfe-commits]

sam-mccall wrote:

I'm sorry that I wasn't able to more usefully reduce the failure cases. When 
such regressions show up, we usually don't have any meaningful context on the 
code. For our own code, we have guidelines to try to limit complexity which 
makes reduction more tractable, but third-party libraries are harder.

For publicly-available code, it's not clear to me how much of the burden should 
fall on people that identify the problem.
I want to do as much of this work as I can, it's difficult to balance the 
urgency of providing some reproducer (it gets hard to push for a fix if we wait 
a week for a good reproducers), the quality of reduction, and other 
work/deadlines. (As mentioned, the timing was difficult this time as this 
landed just before a holiday).

https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Enable C++17 relaxed template template argument matching by default (PR #89807)

[Sam McCall via cfe-commits]

sam-mccall wrote:

@mizvekov Thank you! With that patch, clang not only doesn't crash on stdexec 
with `-frelaxed-template-template-args`, but in fact accepts the code.


https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Enable C++17 relaxed template template argument matching by default (PR #89807)

[Sam McCall via cfe-commits]

sam-mccall wrote:

This commit did three things: A) changed the implementation, B) changed the 
flag default, and C) deprecated the flag.

Since clang now crashes on widely-used, real-world code, can we at least revert 
C, and ideally B until the crashes are fixed?

(It would also have been helpful to separate A and B, automated bisection can't 
tell the difference between the two, and generally we'd deal with crashes in 
default config by reverting the whole commit)

https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Enable C++17 relaxed template template argument matching by default (PR #89807)

[Sam McCall via cfe-commits]

sam-mccall wrote:

Here's a preprocessed file: 
[repro.zip](https://github.com/llvm/llvm-project/files/15250584/repro.zip)

I tried to reduce, and got rid of most of the test code and some of the stdexec 
code, but there's still a lot left.
I hit the end of my timebox on that. Maybe creduce can do better.

https://github.com/llvm/llvm-project/pull/89807
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Enable C++17 relaxed template template argument matching by default (PR #89807)

[Sam McCall via cfe-commits]

sam-mccall wrote:

This patch introduced a crash on code that clang previously accepted (I'm not 
sure whether the code is correct).

The code is 
https://github.com/nvidia/stdexec/tree/467f4a68ee04f3bb4c35e7a5dd13a3419da160cb,
 building `test/stdexec/algos/adaptors/test_stopped_as_optional.cpp` crashes.


Errors prior to crash

```
third_party/stdexec/test/stdexec/algos/adaptors/test_stopped_as_optional.cpp:38:19:
 error: static assertion failed
   38 | static_assert(ex::sender_in);
  |   ^~~
third_party/stdexec/test/stdexec/algos/adaptors/test_stopped_as_optional.cpp:38:19:
 note: because 'ex::sender_in' evaluated to false
./third_party/stdexec/include/stdexec/execution.hpp:824:9: note: because 
'decltype(__impl, 
stdexec::__env::empty_env>()())' (aka 
'stdexec::_ERROR_, 
stdexec::_WITH_META_FUNCTION_T_, 
stdexec::_WITH_TYPES_, 
stdexec::__env::empty_env, 
stdexec::__q,
 stdexec::__q>>') does not satisfy 
'__valid_completion_signatures':
  824 | get_completion_signatures((_Sender&&) __sndr, (_Env&&) __env)
  | ^
./third_party/stdexec/include/stdexec/execution.hpp:273:5: note: because 
'stdexec::_ERROR_, 
stdexec::_WITH_META_FUNCTION_T_, 
stdexec::_WITH_TYPES_, 
stdexec::__env::empty_env, 
stdexec::__q,
 stdexec::__q>>' does not satisfy '__ok'
  273 | __ok<_Completions> && __is_instance_of<_Completions, 
completion_signatures>;
  | ^
./third_party/stdexec/include/stdexec/__detail/__meta.hpp:243:18: note: because 
'__same_as<__ok_t<_ERROR_<_BAD_SUBSTITUTION_<__mstring<76UL>{"The specified 
meta-function could not be evaluated with the types provided."}>, 
_WITH_META_FUNCTION_T_, 
_WITH_TYPES_<__sexpr<(lambda at 
./third_party/stdexec/include/stdexec/__detail/__basic_sender.hpp:594:18)>, 
empty_env, 
__q, 
__q > > >, __msuccess>' evaluated to 
false
  243 |   concept __ok = __same_as<__ok_t<_Arg>, __msuccess>;
  |  ^
./third_party/stdexec/include/stdexec/__detail/__concepts.hpp:51:23: note: 
because '__is_same(stdexec::_ERROR_, 
stdexec::_WITH_META_FUNCTION_T_, 
stdexec::_WITH_TYPES_, 
stdexec::__env::empty_env, 
stdexec::__q,
 stdexec::__q > >, int)' evaluated to 
false
   51 |   concept __same_as = __is_same(_Ap, _Bp);
  |   ^
third_party/stdexec/test/stdexec/algos/adaptors/test_stopped_as_optional.cpp:45:15:
 error: no matching function for call to object of type 'const 
__connect::connect_t'
   45 | auto op = ex::connect(std::move(snd), 
expect_value_receiver{std::optional{}});
  |   ^~~
./third_party/stdexec/include/stdexec/execution.hpp:1471:12: note: candidate 
template ignored: constraints not satisfied [with _Sender = 
__libcpp_remove_reference_t<__sexpr<(lambda at 
./third_party/stdexec/include/stdexec/__detail/__basic_sender.hpp:594:18)> &>, 
_Receiver = expect_value_receiver>]
 1471 |   auto operator()(_Sender&& __sndr, _Receiver&& __rcvr) const
  |^
./third_party/stdexec/include/stdexec/execution.hpp:1468:18: note: because 
'__connectable_with_tag_invoke, 
(anonymous namespace)::expect_value_receiver > >' evaluated to false
 1468 | requires __connectable_with_tag_invoke<_Sender, _Receiver>
  |  ^
./third_party/stdexec/include/stdexec/execution.hpp:1415:7: note: because 
'__connectable_with_tag_invoke_<__tfx_sender<__sexpr<(lambda at 
./third_party/stdexec/include/stdexec/__detail/__basic_sender.hpp:594:18)>, 
expect_value_receiver > >, (anonymous 
namespace)::expect_value_receiver 
> >' evaluated to false
 1415 |   __connectable_with_tag_invoke_<__tfx_sender<_Sender, _Receiver>, 
_Receiver>;
  |   ^
./third_party/stdexec/include/stdexec/execution.hpp:1409:7: note: because 
'sender_in, 
env_of_t > > >' evaluated to 
false
 1409 |   sender_in<_Sender, env_of_t<_Receiver>> && //
  |   ^
./third_party/stdexec/include/stdexec/execution.hpp:824:9: note: because 
'decltype(__impl, 
stdexec::__env::empty_env>()())' (aka 
'_ERROR_{"The 
given type cannot be used as a[...]"}>, 
stdexec::__error__::_WITH_SENDER_>>>, 
stdexec::__error__::_WITH_ENVIRONMENT_>') does not 
satisfy '__valid_completion_signatures':
  824 | get_completion_signatures((_Sender&&) __sndr, (_Env&&) __env)
  | ^
./third_party/stdexec/include/stdexec/execution.hpp:273:5: note: because 
'stdexec::_ERROR_, 
stdexec::__error__::_WITH_SENDER_>>>, 
stdexec::__error__::_WITH_ENVIRONMENT_>' does not 
satisfy '__ok'
  273 | __ok<_Completions> && __is_instance_of<_Completions, 
completion_signatures>;
  | ^
./third_party/stdexec/include/stdexec/__detail/__meta.hpp:243:18: note: because 
'__same_as<__ok_t<_ERROR_<_UNRECOGNIZED_SENDER_TYPE_<__mstring<134UL>{"The 
given type cannot be used as a sender with the given environment because the 
attempt to compute the completion signatures failed."}>, 
_WITH_SENDER_<__sexpr > > >, _WITH_ENVIRONMENT_ > >, 
__msuccess>' evaluated to false
 

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

Thanks, LGTM

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

@@ -1574,6 +1574,7 @@ bool HeaderSearch::ShouldEnterIncludeFile(Preprocessor 
&PP,
 }
   }
 
+  FileInfo.IsLocallyIncluded = true;

sam-mccall wrote:

I'd consider placing this at the end of HandleHeaderIncludeOrImport rather than 
here:

- it looks like there are cases where this function returns true and we don't 
enter the file textually (or at least, it's not *obvious* that there are no 
such cases)
- having a load-bearing side-effect of a "should we do X" query seems 
unexpected and liable to break during refactoring or in edge cases

Concretely I don't see a case where this doesn't work though, up to you.

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

@@ -2057,9 +2065,12 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch 
&HS) {
 // it as a header file (in which case HFI will be null) or if it hasn't
 // changed since it was loaded. Also skip it if it's for a modular header
 // from a different module; in that case, we rely on the module(s)
-// containing the header to provide this information.
+// containing the header to provide this information. Also skip it if it's
+// for any header not from this module that has not been included; in that
+// case, we don't need the information at all.
 const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
-if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
+if (!HFI || (!HFI->isCompilingModuleHeader &&

sam-mccall wrote:

nit: this seems hard to follow, consider splitting up as:

```
if (!HFI)
  continue; // we're not relying on this file at all
if (HFI->isModuleHeader && !HFI->isCompilingModuleHeader)
  continue; // will import HFI from its module
if (!HFI->isCompilingModuleHeader && !PP->alreadyIncluded(*File))
  continue; // unused header needs no description
```

and possibly pulling out as `static shouldDescribeHeader(HFI)` or so?

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

@@ -237,6 +238,8 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module 
*RootModule) {
 CollectIncludingMapsFromAncestors(CurrentModule);
 for (const Module *ImportedModule : CurrentModule->Imports)
   CollectIncludingMapsFromAncestors(ImportedModule);
+for (const Module *UsedModule : CurrentModule->DirectUses)

sam-mccall wrote:

I don't really understand what this change does, and all the tests pass without 
it. Is it related to the rest of this patch?

(Looking at the other collections that happen here, it does seem to "fit". But 
I don't have a really clear idea about this piece).

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

@@ -187,7 +187,8 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module 
*RootModule) {
   continue;
 
 const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
-if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
+if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader) ||
+(HFI->isTextualModuleHeader && !PP.alreadyIncluded(*File)))

sam-mccall wrote:

That makes sense, I don't have any intuition for whether it's necessary or not.

Shortcutting this analysis seems like a useful change of possible, but I think 
restoring the previous behavior is a helpful starting point.

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

@@ -0,0 +1,20 @@
+// This test checks that a module map with a textual header can be marked as

sam-mccall wrote:

This is a useful test, I think there are a couple of other affecting-ness 
properties that are important to test:

 - that a textual header that *is* included means its module is affecting (i.e. 
the include matters)
 - that this textual-affecting-ness doesn't leak outside the module (a module 
depending on B isn't affected by A.modulemap)

I tried to cover these in 
https://github.com/llvm/llvm-project/pull/89729/files#diff-2eed05f9a85bc5a79b9651ee0f23e5f1494e94a2f32e093847aa6dae5ce5d839
 - it might be nice to add these as a second test here (I can do it as a 
followup if you prefer)

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

Thanks! This looks good, much neater than my approach.

I'm interested in whether the DirectUses change is related to this, and would 
like to slap on a couple of tests.

But either way, this looks good and it would be great to have it landed!

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Only modulemaps of included textual headers are affecting (PR #89729)

[Sam McCall via cfe-commits]

@@ -1441,6 +1441,10 @@ void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
   HFI.isCompilingModuleHeader |= isCompilingModuleHeader;
 }
 
+void HeaderSearch::EnteredTextualFile(FileEntryRef File) {
+  getFileInfo(File).isCompilingModuleHeader = true;

sam-mccall wrote:

Oops, I meant to alter that description in this patch, it seems that change got 
lost.

At this point, `isCompilingModuleHeader` is used only for this "is affecting" 
check, and for a heuristic related to translating `#include` into `#import`. 
The latter only cares about modular headers, so it seems reasonable to change 
the semantics for textual headers to something more useful. (But I do need to 
be careful we're only setting this for textual modular headers).

https://github.com/llvm/llvm-project/pull/89729
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

sam-mccall wrote:

> @sam-mccall That makes sense.
> 
> I think one option we have here is to consider all module maps describing a 
> textual header that got included as affecting. I'm concerned that a long 
> chain of textual header includes might again be problematic.

Yeah, that's the option that comes closest to preserving previous behavior, and 
the one that I think most precisely captures the "affecting" semantics - 
including the module maps that were required for the compilation.

I've sent a version of this as https://github.com/llvm/llvm-project/pull/89729, 
based on your first commit here.

> For this particular test, we only need to consider the module maps describing 
> `use`d modules affecting to keep `-fmodules-strict-decluse` happy, which 
> seems like a more precise solution.

I'm not sure this is correct in general: if A uses B which uses C, and both B 
and C are textual, then C.modulemap affects the compilation of A.pcm but won't 
be picked up.

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Only modulemaps of included textual headers are affecting (PR #89729)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall created 
https://github.com/llvm/llvm-project/pull/89729

Prior to this change, modulemaps describing textual headers are considered
to affect the current module whenever HeaderFileInfos for those headers exist.

This wastes creates false dependencies that (among other things) waste
SourceLocation space.

After this change, textual headers don't cause their modulemap to be considered
affecting, unless those textual headers are included (in which case their
modulemap is required e.g. for layering check).


>From 9165d6086e2570198fba1c333d0c9cb9c09037c7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Fri, 19 Apr 2024 12:13:06 -0700
Subject: [PATCH 1/2] [clang][modules] Allow module map files with textual
 header be non-affecting

---
 clang/lib/Serialization/ASTWriter.cpp | 10 ---
 ...e-non-affecting-module-map-files-textual.c | 26 +++
 2 files changed, 32 insertions(+), 4 deletions(-)
 create mode 100644 
clang/test/Modules/prune-non-affecting-module-map-files-textual.c

diff --git a/clang/lib/Serialization/ASTWriter.cpp 
b/clang/lib/Serialization/ASTWriter.cpp
index 8a4b36207c4734..4825c245a4b846 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -187,7 +187,8 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module 
*RootModule) {
   continue;
 
 const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
-if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
+if (!HFI || ((HFI->isModuleHeader || HFI->isTextualModuleHeader) &&
+ !HFI->isCompilingModuleHeader))
   continue;
 
 for (const auto &KH : HS.findResolvedModulesForHeader(*File)) {
@@ -2055,11 +2056,12 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch 
&HS) {
 
 // Get the file info. Skip emitting this file if we have no information on
 // it as a header file (in which case HFI will be null) or if it hasn't
-// changed since it was loaded. Also skip it if it's for a modular header
-// from a different module; in that case, we rely on the module(s)
+// changed since it was loaded. Also skip it if it's for a non-excluded
+// header from a different module; in that case, we rely on the module(s)
 // containing the header to provide this information.
 const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
-if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
+if (!HFI || ((HFI->isModuleHeader || HFI->isTextualModuleHeader) &&
+ !HFI->isCompilingModuleHeader))
   continue;
 
 // Massage the file path into an appropriate form.
diff --git a/clang/test/Modules/prune-non-affecting-module-map-files-textual.c 
b/clang/test/Modules/prune-non-affecting-module-map-files-textual.c
new file mode 100644
index 00..8f8f00560b1834
--- /dev/null
+++ b/clang/test/Modules/prune-non-affecting-module-map-files-textual.c
@@ -0,0 +1,26 @@
+// This test checks that a module map with a textual header can be marked as
+// non-affecting.
+
+// RUN: rm -rf %t && mkdir %t
+// RUN: split-file %s %t
+
+//--- A.modulemap
+module A { textual header "A.h" }
+//--- B.modulemap
+module B { header "B.h" export * }
+//--- A.h
+typedef int A_int;
+//--- B.h
+#include "A.h"
+typedef A_int B_int;
+
+// RUN: %clang_cc1 -fmodules -emit-module %t/A.modulemap -fmodule-name=A -o 
%t/A.pcm \
+// RUN:   -fmodule-map-file=%t/A.modulemap -fmodule-map-file=%t/B.modulemap
+
+// RUN: %clang_cc1 -fmodules -emit-module %t/B.modulemap -fmodule-name=B -o 
%t/B0.pcm \
+// RUN:   -fmodule-map-file=%t/A.modulemap -fmodule-map-file=%t/B.modulemap 
-fmodule-file=%t/A.pcm
+
+// RUN: %clang_cc1 -fmodules -emit-module %t/B.modulemap -fmodule-name=B -o 
%t/B1.pcm \
+// RUN:-fmodule-map-file=%t/B.modulemap 
-fmodule-file=%t/A.pcm
+
+// RUN: diff %t/B0.pcm %t/B1.pcm

>From 0bf2b1583b9b4f5fdbee2e690d075f1b41905532 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Tue, 23 Apr 2024 11:04:32 +0200
Subject: [PATCH 2/2] Track textually included files, and consider their
 modulemaps affecting.

Those modulemaps affect the legality of the inclusions and therefore
the module's compilation.
---
 clang/include/clang/Lex/HeaderSearch.h|  4 ++
 clang/lib/Lex/HeaderSearch.cpp|  4 ++
 clang/lib/Lex/PPDirectives.cpp|  1 +
 ...e-non-affecting-module-map-files-textual.c | 47 ++-
 4 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/clang/include/clang/Lex/HeaderSearch.h 
b/clang/include/clang/Lex/HeaderSearch.h
index c5f90ef4cb3682..c24ef15005f36b 100644
--- a/clang/include/clang/Lex/HeaderSearch.h
+++ b/clang/include/clang/Lex/HeaderSearch.h
@@ -544,6 +544,10 @@ class HeaderSearch {
   bool isImport, bool ModulesEnabled, Module *M,
   bool &IsFirstIncludeOfFile);
 
+  /// Record that we're parsing this file as pa

[clang] [clang][modules] Stop eagerly reading files with diagnostic pragmas (PR #87442)

[Sam McCall via cfe-commits]

sam-mccall wrote:

Yes, it's approximately the same problem. Thanks & sorry for the noise!

(We have a non-clang include-scanner that computes dependencies to ensure 
hermetic builds. The indirect include defeats the include scanner, so we were 
accidentally relying on `` being available for some other reason - 
which I suppose was this one).

https://github.com/llvm/llvm-project/pull/87442
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

sam-mccall wrote:

> I updated the description of this PR, hopefully it makes more sense now. I 
> still need to investigate what goes wrong in 
> "Modules/preprocess-decluse.cpp". It seems that it assumes `%t/b.pcm` embeds 
> the information from "a.modulemap".

I think it should embed that information. The high-level idea here is: the PCM 
should describe the source of the modulemaps that are relevant to build it.

In this test, that means that b.pcm should have a slocentry for a.modulemap, 
because that map was relevant in allowing b.h to include a.h. (If a.pcm 
existed, then it would encode a's module map and we'd use that instead. But as 
it only has textual headers, we don't build a PCM).

The problem is `HFI->isTextualModuleHeader && !HFI->isCompilingModuleHeader` is 
true for `a.h` when building B: it's from a different module, but being built 
as part of this one. We don't have a way to represent this I think: it used to 
be effectively "HFI exists, it's not a modular header, assume we're parsing it" 
but now we're creating the HFI in non-parsing cases.

I can imagine we could change the semantics of `isCompilingModuleHeader` for 
textual headers to be true if the header is being included within a TU for the 
compiling module, rather than included in the modulemap.

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

sam-mccall wrote:

> Is this a pre-existing issue, or did my patch change to make "each textual 
> header gets a `HFI`"?

My best understanding that your patch gave textual headers`HFI`s when the 
module map was loaded, rather than when the header was included. This shouldn't 
have mattered, but for the latent pre-existing bug: this caused the modulemap 
to be "affecting" and thus serialized. (Fixed by this patch)

Your patch changed an early-bailout condition in `markFileModuleHeader` (1426) 
from `if !ModularHeader` to `if ExcludedHeader`, so now we carry on further in 
case of textual headers.
The next line calls `getExistingFileInfo` which is probably fine, and then 
bails out if `ModularHeader` (1430). But with textual headers, we keep going. 
Finally on line 1433 we call `getFileInfo` and the `HFI` is forced to exist.

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

sam-mccall wrote:

> clang's headers all have proper modules now, are you sure you still need A?

We can't use `clang/lib/Headers/module.modulemap`, so we need something to 
describe those headers.

Why can't we? In our build system, it's the **build system's** job to generate 
modulemaps for your libraries, and you don't get to supply your own and build a 
PCM from it. This is a design decision with some far-reaching ups and downs.

It needs a little special bootstrapping logic for the compiler/stdlib headers 
that can't be part of any real library, but that bootstrapping is much simpler 
when we're just injecting one hand-written modulemap, and don't also need the 
build system to produce a PCM.

(I misspoke slightly: `A` is textual headers for clang+stdlib, and `B` is 
modular passthrough headers for the parts that are self-contained. `A` is 
special-cased in the bulid system, `B` is a normal library except that 
everything implicitly depends on it).

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)

[Sam McCall via cfe-commits]

sam-mccall wrote:

Thanks for the pointer to 87848 - reverting that one locally doesn't help 
though, even in combination with applying #89005 and #89428. So this change 
isn't on the critical path to fixing our builds, but still much appreciated and 
will take a look now.

---

Unsurprisingly, the builds that hit this limit are big and slow :-)

Here's a reproducer that seems to capture our case well: 
[modulegen.zip](https://github.com/llvm/llvm-project/files/15045686/modulegen.zip)
`gen.sh` in there creates a bunch of modules under `modules_repro`, 
`modules_repro/build.sh` builds them with `$CLANG`, and `print.sh` shows the 
sloc usage.

Good

```
:2:23: remark: source manager location address space usage: 
[-Rsloc-usage]
2 | #pragma clang __debug sloc_usage
  |   ^
note: 14498B in local locations, 1447248B in locations loaded from AST files, 
for a total of 1461746B (0% of available space)
:1:1: note: file entered 202 times using 1451554B of space
1 | # 1 "" 3
  | ^
:1:1: note: file entered 1 time using 112B of space
1 | #include 
"/usr/local/google/home/sammccall/modulegen/modules_repro/m1/header"
  | ^
```



Bad

```
:2:23: remark: source manager location address space usage: 
[-Rsloc-usage]
2 | #pragma clang __debug sloc_usage
  |   ^
note: 14537B in local locations, 3956449B in locations loaded from AST files, 
for a total of 3970986B (0% of available space)
/usr/local/google/home/sammccall/modulegen/modules_repro/constant.modulemap:1:1:
 note: file entered 100 times using 2505300B of space
1 | module "constant" {
  | ^
:1:1: note: file entered 202 times using 1455493B of space
1 | # 1 "" 3
  | ^
:1:1: note: file entered 1 time using 112B of space
1 | #include 
"/usr/local/google/home/sammccall/modulegen/modules_repro/m1/header"
```




https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

This makes sense to me. It corresponds to our build structure and fixes the new 
build failures we saw after 0cd0aa029647c8d1dba5c3d62f92325576796fa2.

Really appreciate your work on this!

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

sam-mccall wrote:

> Hmm, this will probably only work if you compile `A` into a PCM and then pass 
> it to `B`. This is not how "Modules/preprocess-decluse.cpp" is set up.

In our case we always have a chain A <- B <- C.
A.modulemap can be affecting for B but should not be for C.
(Approximately, A covers approximately clang's headers (textual), B covers the 
standard library (modular). Every other library C sees A.modulemap, 
B.modulemap, B.pcm. There is no PCM for A).

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)

[Sam McCall via cfe-commits]

sam-mccall wrote:

#89441 fixes our build problems, with or without this patch applied.

It's possible this patch makes things better - I haven't checked for actual 
sloc usage yet, just whether the build fails.

So I'll focus on understanding that one and then return here.

(I think I'm close to a sharable reproducer too)

https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] Allow module maps with textual headers to be non-affecting (PR #89441)

[Sam McCall via cfe-commits]

sam-mccall wrote:

I can confirm applying this allows our targets to build again! :tada: 
Thank you, will take a look at the implementation now.

https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)

[Sam McCall via cfe-commits]

sam-mccall wrote:

Hmm, I locally reverted https://github.com/llvm/llvm-project/pull/87849 and 
still see the same issue.
I'll patch #89428 instead, but I don't see how it could do better - it's just 
putting the old and new behavior of #87849 behind a flag, right?

In any case I'll try it, and then work on constructing a reproducer (so far 
I've been mostly treating this as a black box)

https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)

[Sam McCall via cfe-commits]

sam-mccall wrote:

Unfortunately with this patch I'm still seeing the same 
source-location-exhausted error.
I'm going to try to understand why...

https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)

[Sam McCall via cfe-commits]

sam-mccall wrote:

Ilya is out on vacation, I'm able to test this and will get started on that now 
(apologies for delay & thanks for digging into this)

https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix -Wnullability-completeness false-positive in dependent code (PR #88727)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall closed 
https://github.com/llvm/llvm-project/pull/88727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Use TargetOpts from preamble when building ASTs (PR #88381)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.


https://github.com/llvm/llvm-project/pull/88381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Propagate context into stdlib indexing thread (PR #87611)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.


https://github.com/llvm/llvm-project/pull/87611
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang] Migrate clang-rename to OptTable parsing (PR #89167)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall commented:

I'm not sold on the use of OptTable here, and think we should try some 
alternatives. I don't want to be a burden, so I'm happy to try this out if you 
like.

If it's just this tool then it's not that important, but I assume it's not.
There's possible scopes here of busyboxing: llvm (done) -> clang/tools -> 
clang-tools-extra -> out-of-tree tools. My guess is clang/tools is all in 
scope, out-of-tree tools are clearly not, unsure about clang-tools-extra.

busyboxing per se means mostly that tools have to give up on defining 
conflicting symbols, on having different things in the registries between 
tools, on global constructors, and have to live with `main` being a separate 
possibly-generated file. That all seems fine.

OptTable specifically has issues:
 - it can't really be used downstream, so there's a big seam somewhere where 
similar tools do different things, and the in-tree tools aren't good models to 
follow.
 - it apparently requires a lot of new generic boilerplate in every tool, and 
it's opaque xmacro stuff
 - each flag is handled three times (in tablegen, in opt iteration, and as a 
data structure to parse into)
 - it adds a **third** flags mechanism to clang tools, making 
debugging/escaping even harder (today cl::opt covers CommonOptionParser's 
flags, we also have the clang flags consumed via FixedCompilationDatabase)
 - often requires learning a new language (as tablegen isn't widely used in the 
clang-tools world)
 - extra build complexity: cmake/bazel/gn configs gain extra complexity that 
must be synced and debugged
 - risk of subtle changes in flag parsing when migrating tools that currently 
use cl::opt

OptTable doesn't seem to be required:
- the entrypoint is the unopinionated `tool_main(argc,argv**)`, that doesn't 
require all tools to share a flag parsing strategy
- cl::opt can parse from argc/argv fine
- global registration is the main problem, but can be avoided with relatively 
minor source changes (if flags are in main.cpp, but OptTable also requires this)
- global registration can be effectively detected with the use of 
`-Werror=global-constructors` as MLIR does. This will require some cleanup as 
I'm sure we have other violations. (The cleanup does provide some general value 
though.)

concretely I think we could replace existing usage:
```
cl::opt Foo("foo");

int main() {
  ParseCommandLineOptions();
  print(Foo);
}
```

with this:
```
struct Flags {
  cl::opt Foo("foo");
};
const Flags& flags() {
  static Flags flags;
  return flags;
}

int main() {
  flags();
  ParseCommandLineOptions();
  print(flags().Foo);
}
```

There's some additional boilerplate, but it's O(1).
I think the both the migration and the after state would be preferable for the 
tools currently using cl::opt.

https://github.com/llvm/llvm-project/pull/89167
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [clang-tidy] Add new check `modernize-use-designated-initializers` (PR #80541)

[Sam McCall via cfe-commits]

Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= 
Message-ID:
In-Reply-To: 


sam-mccall wrote:

TL;DR: sounds like I should revert the removals/deps in clangd, and let you 
decide how to structure clang-tidy's copy of the code?

`tidy::utils` as a dep has a conceptual layering problem, and a few practical 
ones: it brings in a pile of dependencies that aren't otherwise in the no-tidy 
build (effect: much longer build times), and it sets the wrong expectations 
around performance: that meeting the batch-latency requirements of clang-tidy 
is sufficient, rather than the interactive-latency requirements of clangd. (Not 
a theoretical concern: clangd's *deliberate* deps on clang-tidy have had 
multiple unacceptable performance regressions in the past, which is the cost of 
taking the dep, but not one that needs paying here).

> This were main reason, why it were avoided, as there were no unit tests for 
> this code on clangd side.

This wasn't a library in its own right on the clangd side. If it's a library 
with multiple clients, it needs tests so problems with the library can be 
distinguished from problems with the clients. (I think it would have been nice 
to have fine-grained unittests anyway, but we didn't).

> Why is it such a big deal that a revert needs to be considered?

It's not a big deal, but I thought the most likely fix was to move this into a 
separate library with tests, I wasn't sure if anyone wanted to take that on in 
a hurry, and wanted to get back to a good state.

If tidy owners are happier with just cloning the code, I can revert just the 
changes to clangd.

I would suggest moving the code out out tidy/utils and into the check if that's 
the only place it's going to be tested, but that's up to you.

https://github.com/llvm/llvm-project/pull/80541
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [clang-tidy] Add new check `modernize-use-designated-initializers` (PR #80541)

[Sam McCall via cfe-commits]

Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= ,
Danny =?utf-8?q?Mösch?= 
Message-ID:
In-Reply-To: 


sam-mccall wrote:

Hey folks,

I'm glad the clangd code for designators was useful, but this dep from clangd 
into the middle of tidy::utils isn't really OK - clangd depends on tidy only 
for the purposes of running tidy checks. It would have been appropriate to have 
a clangd reviewer on this.

AFAIK the best place we have for code shared between tools is as a new library 
under `Tooling` - it's not a perfect fit, but there's precedent for this in 
`Tooling/Inclusions`. As a shared library, this would also need unit tests 
distinct from the integration tests in tidy+clangd.

I'm not sure how best to proceed here - if it's possible to agree soon on a fix 
forward, let's do that, otherwise I'd plan to revert next week and you can fix 
as you see fit (extract a shared library, copy the code, etc).

https://github.com/llvm/llvm-project/pull/80541
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [AST][RecoveryExpr] Fix a crash on c89/c90 invalid InitListExpr (#88008) (PR #88014)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

Thanks, this looks like the right fix, and cleaning up DclT while here makes 
sense.

https://github.com/llvm/llvm-project/pull/88014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [AST][RecoveryExpr] Fix a crash on c89/c90 invalid InitListExpr (#88008) (PR #88014)

[Sam McCall via cfe-commits]

@@ -3391,7 +3391,7 @@ class Sema final : public SemaBase {
   bool ConstexprSupported, bool CLinkageMayDiffer);
 
   /// type checking declaration initializers (C99 6.7.8)
-  bool CheckForConstantInitializer(Expr *e, QualType t);
+  bool CheckForConstantInitializer(Expr *Init, unsigned DiagID);

sam-mccall wrote:

you could consider adding a default for DiagID as there's an "obvious" one used 
in most places. Up to you.

https://github.com/llvm/llvm-project/pull/88014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [AST][RecoveryExpr] Fix a crash on c89/c90 invalid InitListExpr (#88008) (PR #88014)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/88014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Propagate context into stdlib indexing thread (PR #87611)

[Sam McCall via cfe-commits]

@@ -112,7 +112,10 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
  // Index outlives TUScheduler (declared first)
  FIndex(FIndex),
  // shared_ptr extends lifetime
- Stdlib(Stdlib)]() mutable {
+ Stdlib(Stdlib),
+ // We have some FS implementations that rely on infomration in
+ // the context.
+ Ctx(Context::current().clone())]() mutable {

sam-mccall wrote:

this doesn't work - need to actually WithContext it inside the body

https://github.com/llvm/llvm-project/pull/87611
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Propagate context into stdlib indexing thread (PR #87611)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/87611
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clangd] Propagate context into stdlib indexing thread (PR #87611)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall commented:

I'm iffy on whether this is semantically right or not, but we need to solve 
this problem somehow, so let's try the most obvious way.

https://github.com/llvm/llvm-project/pull/87611
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix -Wnullability-completeness false-positive in dependent code (PR #88727)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall created 
https://github.com/llvm/llvm-project/pull/88727

The intent was that smart-pointers do not participate in completeness
checks, but we failed to capture dependent `unique_ptr`, which is not
a RecordType but a TemplateSpecializationType.


>From f1bea480b599b431f67df432d130e537d32abe86 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Mon, 15 Apr 2024 14:56:33 +0200
Subject: [PATCH] [clang] fix -Wnullability-completeness false-positive in
 dependent code

The intent was that smart-pointers do not participate in completeness
checks, but we failed to capture dependent `unique_ptr`, which is not
a RecordType but a TemplateSpecializationType.
---
 clang/lib/Sema/SemaType.cpp  | 3 ++-
 clang/test/SemaObjCXX/Inputs/nullability-consistency-smart.h | 4 
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 404c4e8e31b558..6ac8a9433c701b 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -4728,7 +4728,8 @@ static bool shouldHaveNullability(QualType T) {
  // It's unclear whether the pragma's behavior is useful for C++.
  // e.g. treating type-aliases and template-type-parameters differently
  // from types of declarations can be surprising.
- !isa(T->getCanonicalTypeInternal());
+ !isa(
+ T->getCanonicalTypeInternal());
 }
 
 static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
diff --git a/clang/test/SemaObjCXX/Inputs/nullability-consistency-smart.h 
b/clang/test/SemaObjCXX/Inputs/nullability-consistency-smart.h
index a28532e5d71668..5ff974af57f49b 100644
--- a/clang/test/SemaObjCXX/Inputs/nullability-consistency-smart.h
+++ b/clang/test/SemaObjCXX/Inputs/nullability-consistency-smart.h
@@ -5,3 +5,7 @@ void f1(int * _Nonnull);
 void f2(Smart); // OK, not required on smart-pointer types
 using Alias = Smart;
 void f3(Alias);
+
+template  class _Nullable SmartTmpl;
+void f2(SmartTmpl);
+template  void f2(SmartTmpl);

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang][nullability] allow _Nonnull etc on nullable class types (#82705)" (PR #87325)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall closed 
https://github.com/llvm/llvm-project/pull/87325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Revert "Reapply "[clang][nullability] allow _Nonnull etc on nullable class types (#82705)"" (PR #87041)

[Sam McCall via cfe-commits]

sam-mccall wrote:

Sorry Douglas, and thanks for the revert.
(I missed including the new test file in the commit - I'll make sure to run 
through CI when relanding this!)

https://github.com/llvm/llvm-project/pull/87041
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang][nullability] allow _Nonnull etc on nullable class types (#82705)" (PR #87325)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall created 
https://github.com/llvm/llvm-project/pull/87325

This reverts commit 28760b63bbf9e267713957105a8d17091fb0d20e.

The last commit was missing the new testcase, now fixed.


>From dfcb5dd823e2eb4614be6e34369ac703eb87312e Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Tue, 2 Apr 2024 10:45:09 +0200
Subject: [PATCH] Reapply "[clang][nullability] allow _Nonnull etc on nullable
 class types (#82705)"

This reverts commit 28760b63bbf9e267713957105a8d17091fb0d20e.

The last commit was missing the new testcase, now fixed.
---
 clang/docs/ReleaseNotes.rst   | 15 +
 clang/include/clang/Basic/Attr.td |  3 +-
 clang/include/clang/Basic/AttrDocs.td | 25 
 clang/include/clang/Basic/Features.def|  1 +
 clang/include/clang/Parse/Parser.h|  1 +
 clang/include/clang/Sema/Sema.h   |  3 +
 clang/lib/AST/Type.cpp| 29 ++---
 clang/lib/CodeGen/CGCall.cpp  |  3 +-
 clang/lib/CodeGen/CodeGenFunction.cpp |  3 +-
 clang/lib/Parse/ParseDeclCXX.cpp  | 33 +++---
 clang/lib/Sema/SemaAttr.cpp   | 12 
 clang/lib/Sema/SemaChecking.cpp   |  9 +++
 clang/lib/Sema/SemaDecl.cpp   |  4 +-
 clang/lib/Sema/SemaDeclAttr.cpp   | 18 ++
 clang/lib/Sema/SemaInit.cpp   |  5 ++
 clang/lib/Sema/SemaOverload.cpp   |  7 +++
 clang/lib/Sema/SemaTemplate.cpp   |  1 +
 clang/lib/Sema/SemaType.cpp   | 18 --
 clang/test/Sema/nullability.c |  2 +
 clang/test/SemaCXX/nullability.cpp| 62 ++-
 .../Inputs/nullability-consistency-smart.h|  7 +++
 .../SemaObjCXX/nullability-consistency.mm |  1 +
 22 files changed, 233 insertions(+), 29 deletions(-)
 create mode 100644 clang/test/SemaObjCXX/Inputs/nullability-consistency-smart.h

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 76eaf0bf11c303..b2faab1f1525b2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -253,6 +253,21 @@ Attribute Changes in Clang
   added a new extension query ``__has_extension(swiftcc)`` corresponding to the
   ``__attribute__((swiftcc))`` attribute.
 
+- The ``_Nullable`` and ``_Nonnull`` family of type attributes can now apply
+  to certain C++ class types, such as smart pointers:
+  ``void useObject(std::unique_ptr _Nonnull obj);``.
+
+  This works for standard library types including ``unique_ptr``, 
``shared_ptr``,
+  and ``function``. See
+  `the attribute reference documentation 
`_
+  for the full list.
+
+- The ``_Nullable`` attribute can be applied to C++ class declarations:
+  ``template  class _Nullable MySmartPointer {};``.
+
+  This allows the ``_Nullable`` and ``_Nonnull`` family of type attributes to
+  apply to this class.
+
 Improvements to Clang's diagnostics
 ---
 - Clang now applies syntax highlighting to the code snippets it
diff --git a/clang/include/clang/Basic/Attr.td 
b/clang/include/clang/Basic/Attr.td
index 80e607525a0a37..6584460cf5685e 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2178,9 +2178,10 @@ def TypeNonNull : TypeAttr {
   let Documentation = [TypeNonNullDocs];
 }
 
-def TypeNullable : TypeAttr {
+def TypeNullable : DeclOrTypeAttr {
   let Spellings = [CustomKeyword<"_Nullable">];
   let Documentation = [TypeNullableDocs];
+//  let Subjects = SubjectList<[CXXRecord], ErrorDiag>;
 }
 
 def TypeNullableResult : TypeAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td 
b/clang/include/clang/Basic/AttrDocs.td
index 3ea4d676b4f89d..0ca4ea377fc36a 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4151,6 +4151,20 @@ non-underscored keywords. For example:
   @property (assign, nullable) NSView *superview;
   @property (readonly, nonnull) NSArray *subviews;
 @end
+
+As well as built-in pointer types, the nullability attributes can be attached
+to C++ classes marked with the ``_Nullable`` attribute.
+
+The following C++ standard library types are considered nullable:
+``unique_ptr``, ``shared_ptr``, ``auto_ptr``, ``exception_ptr``, ``function``,
+``move_only_function`` and ``coroutine_handle``.
+
+Types should be marked nullable only where the type itself leaves nullability
+ambiguous. For example, ``std::optional`` is not marked ``_Nullable``, because
+``optional _Nullable`` is redundant and ``optional _Nonnull`` is
+not a useful type. ``std::weak_ptr`` is not nullable, because its nullability
+can change with no visible modification, so static annotation is unlikely to be
+unhelpful.
   }];
 }
 
@@ -4185,6 +4199,17 @@ The ``_Nullable`` nullability qualifier indicates that a 
value of the
 int fetch_or_zero(int * _Nullable ptr);
 
 a caller of

[clang] bbbcc1d - Reapply "[clang][nullability] allow _Nonnull etc on nullable class types (#82705)"

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2024-03-28T23:57:09+01:00
New Revision: bbbcc1d99d08855069f4501c896c43a6d4d7b598

URL: 
https://github.com/llvm/llvm-project/commit/bbbcc1d99d08855069f4501c896c43a6d4d7b598
DIFF: 
https://github.com/llvm/llvm-project/commit/bbbcc1d99d08855069f4501c896c43a6d4d7b598.diff

LOG: Reapply "[clang][nullability] allow _Nonnull etc on nullable class types 
(#82705)"

This reverts commit ca4c4a6758d184f209cb5d88ef42ecc011b11642.

This was intended not to introduce new consistency diagnostics for
smart pointer types, but failed to ignore sugar around types when
detecting this.
Fixed and test added.

Added: 


Modified: 
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/Features.def
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/Type.cpp
clang/lib/CodeGen/CGCall.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Sema/SemaAttr.cpp
clang/lib/Sema/SemaChecking.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclAttr.cpp
clang/lib/Sema/SemaInit.cpp
clang/lib/Sema/SemaOverload.cpp
clang/lib/Sema/SemaTemplate.cpp
clang/lib/Sema/SemaType.cpp
clang/test/Sema/nullability.c
clang/test/SemaCXX/nullability.cpp
clang/test/SemaObjCXX/nullability-consistency.mm

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 232de0d7d8bb735..c303eee7be79277 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -253,6 +253,21 @@ Attribute Changes in Clang
   added a new extension query ``__has_extension(swiftcc)`` corresponding to the
   ``__attribute__((swiftcc))`` attribute.
 
+- The ``_Nullable`` and ``_Nonnull`` family of type attributes can now apply
+  to certain C++ class types, such as smart pointers:
+  ``void useObject(std::unique_ptr _Nonnull obj);``.
+
+  This works for standard library types including ``unique_ptr``, 
``shared_ptr``,
+  and ``function``. See
+  `the attribute reference documentation 
`_
+  for the full list.
+
+- The ``_Nullable`` attribute can be applied to C++ class declarations:
+  ``template  class _Nullable MySmartPointer {};``.
+
+  This allows the ``_Nullable`` and ``_Nonnull`` family of type attributes to
+  apply to this class.
+
 Improvements to Clang's diagnostics
 ---
 - Clang now applies syntax highlighting to the code snippets it

diff  --git a/clang/include/clang/Basic/Attr.td 
b/clang/include/clang/Basic/Attr.td
index 80e607525a0a37d..6584460cf5685ea 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2178,9 +2178,10 @@ def TypeNonNull : TypeAttr {
   let Documentation = [TypeNonNullDocs];
 }
 
-def TypeNullable : TypeAttr {
+def TypeNullable : DeclOrTypeAttr {
   let Spellings = [CustomKeyword<"_Nullable">];
   let Documentation = [TypeNullableDocs];
+//  let Subjects = SubjectList<[CXXRecord], ErrorDiag>;
 }
 
 def TypeNullableResult : TypeAttr {

diff  --git a/clang/include/clang/Basic/AttrDocs.td 
b/clang/include/clang/Basic/AttrDocs.td
index 3ea4d676b4f89d3..0ca4ea377fc36ac 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4151,6 +4151,20 @@ non-underscored keywords. For example:
   @property (assign, nullable) NSView *superview;
   @property (readonly, nonnull) NSArray *subviews;
 @end
+
+As well as built-in pointer types, the nullability attributes can be attached
+to C++ classes marked with the ``_Nullable`` attribute.
+
+The following C++ standard library types are considered nullable:
+``unique_ptr``, ``shared_ptr``, ``auto_ptr``, ``exception_ptr``, ``function``,
+``move_only_function`` and ``coroutine_handle``.
+
+Types should be marked nullable only where the type itself leaves nullability
+ambiguous. For example, ``std::optional`` is not marked ``_Nullable``, because
+``optional _Nullable`` is redundant and ``optional _Nonnull`` is
+not a useful type. ``std::weak_ptr`` is not nullable, because its nullability
+can change with no visible modification, so static annotation is unlikely to be
+unhelpful.
   }];
 }
 
@@ -4185,6 +4199,17 @@ The ``_Nullable`` nullability qualifier indicates that a 
value of the
 int fetch_or_zero(int * _Nullable ptr);
 
 a caller of ``fetch_or_zero`` can provide null.
+
+The ``_Nullable`` attribute on classes indicates that the given class can
+represent null values, and so the ``_Nullable``, ``_Nonnull`` etc qualifiers
+make sense for this type. For example:
+
+  .. code-block:: c
+
+class _Nullable ArenaPointer { ... };
+
+ArenaPointer _Nonnull x = ...;
+ArenaPointer _Nullable y = nullptr;
   }];
 }
 

diff  --git a/clang/include/clang/Basic/Fe

[clang] [clang] Invalidate the alias template decl if it has multiple written (PR #85413)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.


https://github.com/llvm/llvm-project/pull/85413
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Add __builtin_start_object_lifetime builtin. (PR #82776)

[Sam McCall via cfe-commits]

@@ -156,6 +156,37 @@ void test_noexcept(int *i) {
 #undef TEST_TYPE
 } // end namespace test_launder
 
+namespace test_start_object_lifetime {
+// The builtin is non-constant.
+constexpr int test_non_constexpr(int i) { // expected-error {{constexpr 
function never produces a constant expression}}
+  __builtin_start_object_lifetime(&i); // expected-note {{subexpression not 
valid in a constant expression}}
+#ifdef CXX11
+  // expected-warning@-2 {{use of this statement in a constexpr function is a 
C++14 extension}}
+#endif
+  return 0;
+}
+
+struct Incomplete; // expected-note {{forward declaration}}
+void test_incomplete(Incomplete *i) {
+   // Requires a complete type
+   __builtin_start_object_lifetime(i); // expected-error {{incomplete type 
'Incomplete' where a complete type is required}}
+}
+
+// The builtin is type-generic.
+#define TEST_TYPE(Ptr, Type) \
+  static_assert(__is_same(decltype(__builtin_launder(Ptr)), Type), "expected 
same type")

sam-mccall wrote:

should this be `__builtin_start_object_lifetime`?

https://github.com/llvm/llvm-project/pull/82776
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Add __builtin_start_object_lifetime builtin. (PR #82776)

[Sam McCall via cfe-commits]

@@ -896,6 +896,12 @@ def Launder : Builtin {
   let Prototype = "void*(void*)";
 }
 
+def StartObjectLifeTime : Builtin {

sam-mccall wrote:

https://github.com/llvm/llvm-project/pull/86512 is the answer here, I think.

https://github.com/llvm/llvm-project/pull/82776
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Add __builtin_start_object_lifetime builtin. (PR #82776)

[Sam McCall via cfe-commits]

sam-mccall wrote:

This has been dormant in part while thinking about the memcpy half, I think.
Something like https://github.com/llvm/llvm-project/pull/86512 solves that well 
but likely needs this change too.

> I am a bit concerned that this does not actually have the desired semantics 
> at all, but @zygoloid seemed to be "happy" with it. I will admit I struggle 
> understanding the motivation of adding a builtin that does...less than it 
> should.

This does something both useful and correct with `-fno-strict-aliasing`. I 
think we're far from alone in building in this mode (in part precisely because 
of existing TBAA soundness questions!)[1].

I think it gets us incrementally closer to fully supporting 
std::start_lifetime_as: if there are specific miscompiles this would produce 
with strict-aliasing enabled, we can now observe them and use them to validate 
e.g. the `llvm.tbaa.fence` proposal. (It looks like what we need here but I 
also don't understand the LLVM change deeply).

> Similarly, do you have plans for the array version of start_lifetime_as?

I think we need a similar parallel version of this, e.g.: 
`__start_dynamic_array_lifetime(T*, size_t) -> T*`.
This seems like a purely mechanical extension that could be done in this patch 
or subsequently. Preferences?

[1] I hope this is the year we can put some work into strict-aliasing and turn 
it on. By coincidence, our best estimate of the overall performance win is 
roughly the same as applying this optimization to one widely-used library...

https://github.com/llvm/llvm-project/pull/82776
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

@@ -5056,6 +5056,8 @@ static bool CheckUnaryTypeTraitTypeCompleteness(Sema &S, 
TypeTrait UTT,
   case UTT_IsStandardLayout:
   case UTT_IsPOD:
   case UTT_IsLiteral:
+  // Clang extension:

sam-mccall wrote:

AFAIK these are all clang extensions (the `__` versions), so I'm not sure this 
comment is clear/helpful

https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, 
KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"
 
 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)

sam-mccall wrote:

naming thought #2:

I'm a little concerned that all other type traits that mention "copy" use it to 
mean "is equivalent to the language-level copy", not "the copy is physically 
possible".

For that reason I would consider e.g. `__is_bitwise_cloneable`.

https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

@@ -2667,6 +2667,29 @@ bool QualType::isTriviallyCopyableType(const ASTContext 
&Context) const {
  /*IsCopyConstructible=*/false);
 }
 
+bool QualType::isBitwiseCopyableType(const ASTContext & Context) const {
+  QualType CanonicalType = getCanonicalType();
+  if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType())
+return false;
+  // Trivially copyable types are bitwise copyable, e.g. scalar types.
+  if (CanonicalType.isTriviallyCopyableType(Context))
+return true;
+
+  if (CanonicalType->isArrayType())
+return Context.getBaseElementType(CanonicalType)
+.isBitwiseCopyableType(Context);
+
+  if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {

sam-mccall wrote:

this seems to exclude very few types, like reference members. Is this the right 
approach?


https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+
+// Scalar types are bitwise copyable.
+static_assert(__is_bitwise_copyable(int));
+static_assert(__is_bitwise_copyable(int*));
+// array
+static_assert(__is_bitwise_copyable(int[10]));
+
+
+struct Forward; // expected-note 2{{forward declaration of 'Forward'}}
+static_assert(!__is_bitwise_copyable(Forward)); // expected-error {{incomplete 
type 'Forward' used in type trait expression}}
+
+struct Foo { int a; };
+static_assert(__is_bitwise_copyable(Foo));
+
+struct DynamicClass { virtual int Foo(); };
+static_assert(__is_bitwise_copyable(DynamicClass));
+
+template 
+void TemplateFunction() {
+  static_assert(__is_bitwise_copyable(T)); // expected-error {{incomplete type 
'Forward' used in type trait expression}}
+}
+void CallTemplateFunc() {
+  TemplateFunction(); // expected-note {{in instantiation of function 
template specialization}}
+  TemplateFunction();
+}

sam-mccall wrote:

if there are some class types not intended to be bitwise copyable (like `struct 
{ int &x; }`), we should have negative test cases too

https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

@@ -2667,6 +2667,29 @@ bool QualType::isTriviallyCopyableType(const ASTContext 
&Context) const {
  /*IsCopyConstructible=*/false);
 }
 
+bool QualType::isBitwiseCopyableType(const ASTContext & Context) const {
+  QualType CanonicalType = getCanonicalType();
+  if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType())

sam-mccall wrote:

should the answer here be dependent if the type is dependent?
similarly should incomplete be an error?

what do other traits do?

https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, 
KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"
 
 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)

sam-mccall wrote:

naming thought #1 (sorry): 

if we're going to use "bit" here, you might consider e.g. `__is_bit_copyable` 
rather than "bitwise", for consistency with `std::bit_cast` which is closely 
related to this concept.

(In a vacuum I do like "bitwise" better, though "bytewise_copyable" seems more 
accurate: the copy (noun) is bit-for-bit but the copy (verb) is byte-by-byte)

https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, 
KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"
 
 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)

sam-mccall wrote:

why c++ only?

https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, 
KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"
 
 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)

sam-mccall wrote:

this new trait should be documented in `docs/LanguageExtensions`.

The documentation needs to explicitly mention whether or not the user code 
needs to perform a "start lifetime" operation to incarnate the resulting bytes. 

https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

@@ -2667,6 +2667,29 @@ bool QualType::isTriviallyCopyableType(const ASTContext 
&Context) const {
  /*IsCopyConstructible=*/false);
 }
 
+bool QualType::isBitwiseCopyableType(const ASTContext & Context) const {
+  QualType CanonicalType = getCanonicalType();
+  if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType())
+return false;
+  // Trivially copyable types are bitwise copyable, e.g. scalar types.
+  if (CanonicalType.isTriviallyCopyableType(Context))
+return true;
+
+  if (CanonicalType->isArrayType())
+return Context.getBaseElementType(CanonicalType)
+.isBitwiseCopyableType(Context);
+
+  if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
+for (auto *const Field : RD->fields()) {
+  QualType T = Context.getBaseElementType(Field->getType());

sam-mccall wrote:

we're missing base classes and such

https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Implement a bitwise_copyable builtin type trait. (PR #86512)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall commented:

I'm in favour of this, but we should have someone less-attached to it sign off 
on at least the basic design.

One question this doesn't explicitly address: after memcpying, does user code 
need to bless the bits somehow to start the new object's lifetime?

I think the answer should probably be yes, even if blessing is a no-op for now. 
https://github.com/llvm/llvm-project/pull/82776 seems suitable.

`__is_trivially_copyable` doesn't require this, but the types it applies to are 
pretty limited in terms of lifetime concerns, so I think it's OK to draw a 
distinction.

One argument for this is with no "bless" barrier, I don't understand exactly 
where the line is drawn for strict-aliasing purposes:
 - if we can memcpy an object A into a buffer B
 - then surely we can copy it ourselves byte-by-byte
 - and we can then immediately copy it back from B to A, if the destructor is 
trivial
 - now we get to treat it as a different type
 Here A's bytes didn't change - is the write to B important? Do we have to 
write the bytes sequentially for this transmutation of A to work, or can we 
reuse a single byte variable?

This all seems weird and like we're reinventing implicit-lifetime types. That's 
by necessity a weird feature, but the weirdness is not needed here.

https://github.com/llvm/llvm-project/pull/86512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [AST] Print the "aggregate" for aggregate deduction guide decl. (PR #84018)

[Sam McCall via cfe-commits]

@@ -1990,6 +1990,18 @@ void TextNodeDumper::VisitFunctionDecl(const 
FunctionDecl *D) {
   }
 }
 
+void TextNodeDumper::VisitCXXDeductionGuideDecl(const CXXDeductionGuideDecl 
*D) {
+  VisitFunctionDecl(D);
+  switch (D->getDeductionCandidateKind()) {
+  case DeductionCandidate::Normal:
+  case DeductionCandidate::Copy:

sam-mccall wrote:

while here, I think adding "copy" might also make sense?

(not sure, i'm not immersed in CTAD stuff, so up to you)

https://github.com/llvm/llvm-project/pull/84018
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [AST] Print the "aggregate" for aggregate deduction guide decl. (PR #84018)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.


https://github.com/llvm/llvm-project/pull/84018
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [AST] Print the "aggregate" for aggregate deduction guide decl. (PR #84018)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/84018
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] Fix crash when analyzing a coroutine (PR #85957)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/85957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] Fix crash when analyzing a coroutine (PR #85957)

[Sam McCall via cfe-commits]

@@ -4628,6 +4662,21 @@ TEST(TransferTest, DoesNotCrashOnUnionThisExpr) {
   LangStandard::lang_cxx17, /*ApplyBuiltinTransfer=*/true, "operator=");
 }
 
+TEST(TransferTest, DoesNotCrashOnNullChildren) {
+  std::string Code = (CoroutineLibrary + R"cc(
+task foo() noexcept {
+  co_return;
+}
+  )cc").str();
+  // This is a crash regression test when calling `AdornedCFG::build` on a
+  // statement (in this case, the `CoroutineBodyStmt`) with null children.
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &,
+ ASTContext &) {},
+  LangStandard::lang_cxx20, /*ApplyBuiltinTransfer=*/true, "foo");

sam-mccall wrote:

(nit: just call the function "target" and skip naming it here)

https://github.com/llvm/llvm-project/pull/85957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][dataflow] Fix crash when analyzing a coroutine (PR #85957)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.


https://github.com/llvm/llvm-project/pull/85957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] ca4c4a6 - Revert "[clang][nullability] allow _Nonnull etc on nullable class types (#82705)"

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2024-03-15T21:55:37+01:00
New Revision: ca4c4a6758d184f209cb5d88ef42ecc011b11642

URL: 
https://github.com/llvm/llvm-project/commit/ca4c4a6758d184f209cb5d88ef42ecc011b11642
DIFF: 
https://github.com/llvm/llvm-project/commit/ca4c4a6758d184f209cb5d88ef42ecc011b11642.diff

LOG: Revert "[clang][nullability] allow _Nonnull etc on nullable class types 
(#82705)"

This reverts commit 92a09c0165b87032e1bd05020a78ed845cf35661.

This is triggering a bunch of new -Wnullability-completeness warnings
in code with existing raw pointer nullability annotations.

The intent was the new nullability locations wouldn't affect those
warnings, so this is a bug at least for now.

Added: 


Modified: 
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/Features.def
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/Type.cpp
clang/lib/CodeGen/CGCall.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Sema/SemaAttr.cpp
clang/lib/Sema/SemaChecking.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclAttr.cpp
clang/lib/Sema/SemaInit.cpp
clang/lib/Sema/SemaOverload.cpp
clang/lib/Sema/SemaTemplate.cpp
clang/lib/Sema/SemaType.cpp
clang/test/Sema/nullability.c
clang/test/SemaCXX/nullability.cpp

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e1743368b157e0..623a4b3c18bb1a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -204,21 +204,6 @@ Attribute Changes in Clang
   and each must be a positive integer when provided. The parameter ``x`` is 
required, while ``y`` and
   ``z`` are optional with default value of 1.
 
-- The ``_Nullable`` and ``_Nonnull`` family of type attributes can now apply
-  to certain C++ class types, such as smart pointers:
-  ``void useObject(std::unique_ptr _Nonnull obj);``.
-
-  This works for standard library types including ``unique_ptr``, 
``shared_ptr``,
-  and ``function``. See
-  `the attribute reference documentation 
`_
-  for the full list.
-
-- The ``_Nullable`` attribute can be applied to C++ class declarations:
-  ``template  class _Nullable MySmartPointer {};``.
-
-  This allows the ``_Nullable`` and ``_Nonnull`` family of type attributes to
-  apply to this class.
-
 Improvements to Clang's diagnostics
 ---
 - Clang now applies syntax highlighting to the code snippets it

diff  --git a/clang/include/clang/Basic/Attr.td 
b/clang/include/clang/Basic/Attr.td
index 24bc06476b1f9c..67d87eca16ede8 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2178,10 +2178,9 @@ def TypeNonNull : TypeAttr {
   let Documentation = [TypeNonNullDocs];
 }
 
-def TypeNullable : DeclOrTypeAttr {
+def TypeNullable : TypeAttr {
   let Spellings = [CustomKeyword<"_Nullable">];
   let Documentation = [TypeNullableDocs];
-//  let Subjects = SubjectList<[CXXRecord], ErrorDiag>;
 }
 
 def TypeNullableResult : TypeAttr {

diff  --git a/clang/include/clang/Basic/AttrDocs.td 
b/clang/include/clang/Basic/AttrDocs.td
index 075324a213ff78..d61f96ade557d5 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4151,20 +4151,6 @@ non-underscored keywords. For example:
   @property (assign, nullable) NSView *superview;
   @property (readonly, nonnull) NSArray *subviews;
 @end
-
-As well as built-in pointer types, the nullability attributes can be attached
-to C++ classes marked with the ``_Nullable`` attribute.
-
-The following C++ standard library types are considered nullable:
-``unique_ptr``, ``shared_ptr``, ``auto_ptr``, ``exception_ptr``, ``function``,
-``move_only_function`` and ``coroutine_handle``.
-
-Types should be marked nullable only where the type itself leaves nullability
-ambiguous. For example, ``std::optional`` is not marked ``_Nullable``, because
-``optional _Nullable`` is redundant and ``optional _Nonnull`` is
-not a useful type. ``std::weak_ptr`` is not nullable, because its nullability
-can change with no visible modification, so static annotation is unlikely to be
-unhelpful.
   }];
 }
 
@@ -4199,17 +4185,6 @@ The ``_Nullable`` nullability qualifier indicates that a 
value of the
 int fetch_or_zero(int * _Nullable ptr);
 
 a caller of ``fetch_or_zero`` can provide null.
-
-The ``_Nullable`` attribute on classes indicates that the given class can
-represent null values, and so the ``_Nullable``, ``_Nonnull`` etc qualifiers
-make sense for this type. For example:
-
-  .. code-block:: c
-
-class _Nullable ArenaPointer { ... };
-
-ArenaPointer _Nonnull x = ...;
-ArenaPointer _Nullable y = nullptr;
   }];
 }
 

diff

[clang] [clang] Fix Clang language extension documentation markup for __builtin_arm_trap. (PR #85310)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

Thank you, and sorry for piling on extra errors...

https://github.com/llvm/llvm-project/pull/85310
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][nullability] allow _Nonnull etc on nullable class types (PR #82705)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall closed 
https://github.com/llvm/llvm-project/pull/82705
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][nullability] allow _Nonnull etc on nullable class types (PR #82705)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall updated 
https://github.com/llvm/llvm-project/pull/82705

>From ebf37038879e6b7ea2a315f267dc1dfe10a12c41 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Thu, 22 Feb 2024 16:00:44 +0100
Subject: [PATCH 1/5] [clang][nullability] allow _Nonnull etc on nullable class
 types

This enables clang and external nullability checkers to make use of
these annotations on nullable C++ class types like unique_ptr.

These types are recognized by the presence of the _Nullable attribute.
Nullable standard library types implicitly receive this attribute.

Existing static warnings for raw pointers are extended to smart pointers:

- nullptr used as return value or argument for non-null functions
  (`-Wnonnull`)
- assigning or initializing nonnull variables with nullable values
  (`-Wnullable-to-nonnull-conversion`)

It doesn't implicitly add these attributes based on the assume_nonnull
pragma, nor warn on missing attributes where the pragma would apply them.
I'm not confident that the pragma's current behavior will work well for
C++ (where type-based metaprogramming is much more common than C/ObjC).
We'd like to revisit this once we have more implementation experience.

Support can be detected as `__has_feature(nullability_on_classes)`.
This is needed for back-compatibility, as previously clang would issue a
hard error when _Nullable appears on a smart pointer.

UBSan's `-fsanitize=nullability` will not check smart-pointer types.
It can be made to do so by synthesizing calls to `operator bool`, but
that's left for future work.
---
 clang/include/clang/Basic/Attr.td  |  3 +-
 clang/include/clang/Basic/AttrDocs.td  | 16 +++
 clang/include/clang/Basic/Features.def |  1 +
 clang/include/clang/Parse/Parser.h |  1 +
 clang/include/clang/Sema/Sema.h| 14 --
 clang/lib/AST/Type.cpp | 29 +++-
 clang/lib/CodeGen/CGCall.cpp   |  3 +-
 clang/lib/CodeGen/CodeGenFunction.cpp  |  3 +-
 clang/lib/Parse/ParseDeclCXX.cpp   | 33 ++
 clang/lib/Sema/SemaAttr.cpp| 12 +
 clang/lib/Sema/SemaChecking.cpp|  9 
 clang/lib/Sema/SemaDecl.cpp|  4 +-
 clang/lib/Sema/SemaDeclAttr.cpp| 18 
 clang/lib/Sema/SemaInit.cpp|  5 +++
 clang/lib/Sema/SemaOverload.cpp|  7 +++
 clang/lib/Sema/SemaTemplate.cpp|  1 +
 clang/lib/Sema/SemaType.cpp| 18 ++--
 clang/test/SemaCXX/nullability.cpp | 62 +-
 18 files changed, 196 insertions(+), 43 deletions(-)

diff --git a/clang/include/clang/Basic/Attr.td 
b/clang/include/clang/Basic/Attr.td
index 67d87eca16ede8..24bc06476b1f9c 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2178,9 +2178,10 @@ def TypeNonNull : TypeAttr {
   let Documentation = [TypeNonNullDocs];
 }
 
-def TypeNullable : TypeAttr {
+def TypeNullable : DeclOrTypeAttr {
   let Spellings = [CustomKeyword<"_Nullable">];
   let Documentation = [TypeNullableDocs];
+//  let Subjects = SubjectList<[CXXRecord], ErrorDiag>;
 }
 
 def TypeNullableResult : TypeAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td 
b/clang/include/clang/Basic/AttrDocs.td
index d61f96ade557d5..3170e8d6df6eec 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4151,6 +4151,11 @@ non-underscored keywords. For example:
   @property (assign, nullable) NSView *superview;
   @property (readonly, nonnull) NSArray *subviews;
 @end
+
+As well as built-in pointer types, ithe nullability attributes can be attached
+to nullable types from the C++ standard library such as ``std::unique_ptr`` and
+``std::function``, as well as C++ classes marked with the ``_Nullable``
+attribute.
   }];
 }
 
@@ -4185,6 +4190,17 @@ The ``_Nullable`` nullability qualifier indicates that a 
value of the
 int fetch_or_zero(int * _Nullable ptr);
 
 a caller of ``fetch_or_zero`` can provide null.
+
+The ``_Nullable`` attribute on classes indicates that the given class can
+represent null values, and so the ``_Nullable``, ``_Nonnull`` etc qualifiers
+make sense for this type. For example:
+
+  .. code-block:: c
+
+class _Nullable ArenaPointer { ... };
+
+ArenaPointer _Nonnull x = ...;
+ArenaPointer _Nullable y = nullptr;
   }];
 }
 
diff --git a/clang/include/clang/Basic/Features.def 
b/clang/include/clang/Basic/Features.def
index 5fad5fc3623cb6..7c0755b7318306 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -94,6 +94,7 @@ EXTENSION(define_target_os_macros,
 FEATURE(enumerator_attributes, true)
 FEATURE(nullability, true)
 FEATURE(nullability_on_arrays, true)
+FEATURE(nullability_on_classes, true)
 FEATURE(nullability_nullable_result, true)
 FEATURE(memory_sanitizer,
 LangOpts.Sanitize.hasOneOf(SanitizerKind::Memory |
diff --git a/clang/include/clang/Parse/Parser.h 
b/clang/include/clang/Parse/Parser.h
index 64e031d5094c74..1

[clang] [clang][nullability] allow _Nonnull etc on nullable class types (PR #82705)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall updated 
https://github.com/llvm/llvm-project/pull/82705

>From eccc46840e343e7ba15200cd4b81316a51c46943 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Thu, 22 Feb 2024 16:00:44 +0100
Subject: [PATCH 1/4] [clang][nullability] allow _Nonnull etc on nullable class
 types

This enables clang and external nullability checkers to make use of
these annotations on nullable C++ class types like unique_ptr.

These types are recognized by the presence of the _Nullable attribute.
Nullable standard library types implicitly receive this attribute.

Existing static warnings for raw pointers are extended to smart pointers:

- nullptr used as return value or argument for non-null functions
  (`-Wnonnull`)
- assigning or initializing nonnull variables with nullable values
  (`-Wnullable-to-nonnull-conversion`)

It doesn't implicitly add these attributes based on the assume_nonnull
pragma, nor warn on missing attributes where the pragma would apply them.
I'm not confident that the pragma's current behavior will work well for
C++ (where type-based metaprogramming is much more common than C/ObjC).
We'd like to revisit this once we have more implementation experience.

Support can be detected as `__has_feature(nullability_on_classes)`.
This is needed for back-compatibility, as previously clang would issue a
hard error when _Nullable appears on a smart pointer.

UBSan's `-fsanitize=nullability` will not check smart-pointer types.
It can be made to do so by synthesizing calls to `operator bool`, but
that's left for future work.
---
 clang/include/clang/Basic/Attr.td  |  3 +-
 clang/include/clang/Basic/AttrDocs.td  | 16 +++
 clang/include/clang/Basic/Features.def |  1 +
 clang/include/clang/Parse/Parser.h |  1 +
 clang/include/clang/Sema/Sema.h|  3 ++
 clang/lib/AST/Type.cpp | 29 +++-
 clang/lib/CodeGen/CGCall.cpp   |  3 +-
 clang/lib/CodeGen/CodeGenFunction.cpp  |  3 +-
 clang/lib/Parse/ParseDeclCXX.cpp   | 33 ++
 clang/lib/Sema/SemaAttr.cpp| 12 +
 clang/lib/Sema/SemaChecking.cpp|  9 
 clang/lib/Sema/SemaDecl.cpp|  4 +-
 clang/lib/Sema/SemaDeclAttr.cpp| 18 
 clang/lib/Sema/SemaInit.cpp|  5 +++
 clang/lib/Sema/SemaOverload.cpp|  7 +++
 clang/lib/Sema/SemaTemplate.cpp|  1 +
 clang/lib/Sema/SemaType.cpp| 18 ++--
 clang/test/SemaCXX/nullability.cpp | 62 +-
 18 files changed, 199 insertions(+), 29 deletions(-)

diff --git a/clang/include/clang/Basic/Attr.td 
b/clang/include/clang/Basic/Attr.td
index fa191c7378dba4..f75aa126cbb939 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2156,9 +2156,10 @@ def TypeNonNull : TypeAttr {
   let Documentation = [TypeNonNullDocs];
 }
 
-def TypeNullable : TypeAttr {
+def TypeNullable : DeclOrTypeAttr {
   let Spellings = [CustomKeyword<"_Nullable">];
   let Documentation = [TypeNullableDocs];
+//  let Subjects = SubjectList<[CXXRecord], ErrorDiag>;
 }
 
 def TypeNullableResult : TypeAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td 
b/clang/include/clang/Basic/AttrDocs.td
index b96fbddd51154c..8c248fb052f19e 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4096,6 +4096,11 @@ non-underscored keywords. For example:
   @property (assign, nullable) NSView *superview;
   @property (readonly, nonnull) NSArray *subviews;
 @end
+
+As well as built-in pointer types, ithe nullability attributes can be attached
+to nullable types from the C++ standard library such as ``std::unique_ptr`` and
+``std::function``, as well as C++ classes marked with the ``_Nullable``
+attribute.
   }];
 }
 
@@ -4130,6 +4135,17 @@ The ``_Nullable`` nullability qualifier indicates that a 
value of the
 int fetch_or_zero(int * _Nullable ptr);
 
 a caller of ``fetch_or_zero`` can provide null.
+
+The ``_Nullable`` attribute on classes indicates that the given class can
+represent null values, and so the ``_Nullable``, ``_Nonnull`` etc qualifiers
+make sense for this type. For example:
+
+  .. code-block:: c
+
+class _Nullable ArenaPointer { ... };
+
+ArenaPointer _Nonnull x = ...;
+ArenaPointer _Nullable y = nullptr;
   }];
 }
 
diff --git a/clang/include/clang/Basic/Features.def 
b/clang/include/clang/Basic/Features.def
index 5fad5fc3623cb6..7c0755b7318306 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -94,6 +94,7 @@ EXTENSION(define_target_os_macros,
 FEATURE(enumerator_attributes, true)
 FEATURE(nullability, true)
 FEATURE(nullability_on_arrays, true)
+FEATURE(nullability_on_classes, true)
 FEATURE(nullability_nullable_result, true)
 FEATURE(memory_sanitizer,
 LangOpts.Sanitize.hasOneOf(SanitizerKind::Memory |
diff --git a/clang/include/clang/Parse/Parser.h 
b/clang/include/clang/Parse/Parser.h
index 071520f535bc95..3d858

[clang] [clang-tools-extra] [concepts] Preserve the FoundDecl of ConceptReference properly (PR #85032)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

Very nice, thanks!

https://github.com/llvm/llvm-project/pull/85032
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

This direction looks good to me, mostly I just have nits.

The one major change I'd consider before landing is to find a way to avoid the 
verbosity regression cor3ntin mentions for `Diag()` and friends. Notational 
regressions within Sema itself may be as important as those of its clients, as 
it's so much code.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

@@ -63,17 +67,17 @@ void Sema::ActOnOpenACCConstruct(OpenACCDirectiveKind K,
 // here as these constructs do not take any arguments.
 break;
   default:
-Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;
+SemaRef.Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;

sam-mccall wrote:

Agreed. Is there a better way than this?

```
class SemaComponent {
  Sema &S;
protected:
  SemaComponent(Sema &S) : S(S) {}
  Sema& sema() { return S; }
  SemaDiagnosticBuilder Diag(...) { return sema().Diag(...); }
  // ...
};

class SemaOpenACC : public SemaComponent {
public:
  using SemaComponent::SemaComponent;
  ...
}
```

Accessing `sema()` would be a marker of crossing component boundaries, which is 
worthy of some scrutiny.

This requires some agreement on what common functionality is. On the other hand 
it forces some discussion/review of this definition which seems useful.

It requires SemaOpenACC.h to depend on Sema.h (which personally I think is 
reasonable, based on what clients will do).

It also requires a bunch of boilerplate, which is sad.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

@@ -0,0 +1,67 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// 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
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class Sema;

sam-mccall wrote:

Do we expect that clients realistically use SemaOpenACC but not 
Sema::OpenACC()? Do we want to encourage clients to accept/store `SemaFoo&` and 
`SemaBar&` instead of `Sema&`?

I think we should accept that at least for now, clients will and should still 
use `Sema` as the god-object, and that their benefit is not having to pay the 
compile time of `#include "SemaUnrelated.h"` or the human time of reading its 
contents.
This is the pattern that client code is being migrated to in this patch, and 
it's much more amenable to mechanical migration.

In which case, I think we should `#include "Sema.h"` instead of a forward 
declaration. Forward declarations should be used in the cases where they 
matter, but otherwise humans waste effort trying to preserve this unneeded 
property. (Also, `#including "Sema.h"` makes it clear that the reverse 
inclusion is not allowed for layering reasons, which is much more important).

(Again, nitpicking because this is a precedent)

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {

sam-mccall wrote:

nit: per style guide, this should be `openACC()` (lowercase O), and it should 
be `getOpenAcc()` (verb phrase).

Personally, I think the verb-phrase rule when applied to getters is harmful 
enough to be worth ignoring (preferring [name according to 
side-effects](https://www.swift.org/documentation/api-design-guidelines/#name-according-to-side-effects)).
 So I'd be happiest with `openACC()`, but also fine with `getOpenACC()`.

(Picking this nit because I suspect we're going to have a bunch of these, and 
they should be consistent.)

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Note that optnone and target attributes do not apply to nested functions (PR #82815)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

Makes sense to me. If this ends up applying to lots of attributes then maybe we 
should hoist the text somewhere common, but agree that it doesn't seem to.

https://github.com/llvm/llvm-project/pull/82815
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][nullability] allow _Nonnull etc on nullable class types (PR #82705)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall updated 
https://github.com/llvm/llvm-project/pull/82705

>From eccc46840e343e7ba15200cd4b81316a51c46943 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Thu, 22 Feb 2024 16:00:44 +0100
Subject: [PATCH 1/3] [clang][nullability] allow _Nonnull etc on nullable class
 types

This enables clang and external nullability checkers to make use of
these annotations on nullable C++ class types like unique_ptr.

These types are recognized by the presence of the _Nullable attribute.
Nullable standard library types implicitly receive this attribute.

Existing static warnings for raw pointers are extended to smart pointers:

- nullptr used as return value or argument for non-null functions
  (`-Wnonnull`)
- assigning or initializing nonnull variables with nullable values
  (`-Wnullable-to-nonnull-conversion`)

It doesn't implicitly add these attributes based on the assume_nonnull
pragma, nor warn on missing attributes where the pragma would apply them.
I'm not confident that the pragma's current behavior will work well for
C++ (where type-based metaprogramming is much more common than C/ObjC).
We'd like to revisit this once we have more implementation experience.

Support can be detected as `__has_feature(nullability_on_classes)`.
This is needed for back-compatibility, as previously clang would issue a
hard error when _Nullable appears on a smart pointer.

UBSan's `-fsanitize=nullability` will not check smart-pointer types.
It can be made to do so by synthesizing calls to `operator bool`, but
that's left for future work.
---
 clang/include/clang/Basic/Attr.td  |  3 +-
 clang/include/clang/Basic/AttrDocs.td  | 16 +++
 clang/include/clang/Basic/Features.def |  1 +
 clang/include/clang/Parse/Parser.h |  1 +
 clang/include/clang/Sema/Sema.h|  3 ++
 clang/lib/AST/Type.cpp | 29 +++-
 clang/lib/CodeGen/CGCall.cpp   |  3 +-
 clang/lib/CodeGen/CodeGenFunction.cpp  |  3 +-
 clang/lib/Parse/ParseDeclCXX.cpp   | 33 ++
 clang/lib/Sema/SemaAttr.cpp| 12 +
 clang/lib/Sema/SemaChecking.cpp|  9 
 clang/lib/Sema/SemaDecl.cpp|  4 +-
 clang/lib/Sema/SemaDeclAttr.cpp| 18 
 clang/lib/Sema/SemaInit.cpp|  5 +++
 clang/lib/Sema/SemaOverload.cpp|  7 +++
 clang/lib/Sema/SemaTemplate.cpp|  1 +
 clang/lib/Sema/SemaType.cpp| 18 ++--
 clang/test/SemaCXX/nullability.cpp | 62 +-
 18 files changed, 199 insertions(+), 29 deletions(-)

diff --git a/clang/include/clang/Basic/Attr.td 
b/clang/include/clang/Basic/Attr.td
index fa191c7378dba4..f75aa126cbb939 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2156,9 +2156,10 @@ def TypeNonNull : TypeAttr {
   let Documentation = [TypeNonNullDocs];
 }
 
-def TypeNullable : TypeAttr {
+def TypeNullable : DeclOrTypeAttr {
   let Spellings = [CustomKeyword<"_Nullable">];
   let Documentation = [TypeNullableDocs];
+//  let Subjects = SubjectList<[CXXRecord], ErrorDiag>;
 }
 
 def TypeNullableResult : TypeAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td 
b/clang/include/clang/Basic/AttrDocs.td
index b96fbddd51154c..8c248fb052f19e 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4096,6 +4096,11 @@ non-underscored keywords. For example:
   @property (assign, nullable) NSView *superview;
   @property (readonly, nonnull) NSArray *subviews;
 @end
+
+As well as built-in pointer types, ithe nullability attributes can be attached
+to nullable types from the C++ standard library such as ``std::unique_ptr`` and
+``std::function``, as well as C++ classes marked with the ``_Nullable``
+attribute.
   }];
 }
 
@@ -4130,6 +4135,17 @@ The ``_Nullable`` nullability qualifier indicates that a 
value of the
 int fetch_or_zero(int * _Nullable ptr);
 
 a caller of ``fetch_or_zero`` can provide null.
+
+The ``_Nullable`` attribute on classes indicates that the given class can
+represent null values, and so the ``_Nullable``, ``_Nonnull`` etc qualifiers
+make sense for this type. For example:
+
+  .. code-block:: c
+
+class _Nullable ArenaPointer { ... };
+
+ArenaPointer _Nonnull x = ...;
+ArenaPointer _Nullable y = nullptr;
   }];
 }
 
diff --git a/clang/include/clang/Basic/Features.def 
b/clang/include/clang/Basic/Features.def
index 5fad5fc3623cb6..7c0755b7318306 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -94,6 +94,7 @@ EXTENSION(define_target_os_macros,
 FEATURE(enumerator_attributes, true)
 FEATURE(nullability, true)
 FEATURE(nullability_on_arrays, true)
+FEATURE(nullability_on_classes, true)
 FEATURE(nullability_nullable_result, true)
 FEATURE(memory_sanitizer,
 LangOpts.Sanitize.hasOneOf(SanitizerKind::Memory |
diff --git a/clang/include/clang/Parse/Parser.h 
b/clang/include/clang/Parse/Parser.h
index 071520f535bc95..3d858

[clang] [clang][nullability] allow _Nonnull etc on nullable class types (PR #82705)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall updated 
https://github.com/llvm/llvm-project/pull/82705

>From eccc46840e343e7ba15200cd4b81316a51c46943 Mon Sep 17 00:00:00 2001
From: Sam McCall 
Date: Thu, 22 Feb 2024 16:00:44 +0100
Subject: [PATCH 1/2] [clang][nullability] allow _Nonnull etc on nullable class
 types

This enables clang and external nullability checkers to make use of
these annotations on nullable C++ class types like unique_ptr.

These types are recognized by the presence of the _Nullable attribute.
Nullable standard library types implicitly receive this attribute.

Existing static warnings for raw pointers are extended to smart pointers:

- nullptr used as return value or argument for non-null functions
  (`-Wnonnull`)
- assigning or initializing nonnull variables with nullable values
  (`-Wnullable-to-nonnull-conversion`)

It doesn't implicitly add these attributes based on the assume_nonnull
pragma, nor warn on missing attributes where the pragma would apply them.
I'm not confident that the pragma's current behavior will work well for
C++ (where type-based metaprogramming is much more common than C/ObjC).
We'd like to revisit this once we have more implementation experience.

Support can be detected as `__has_feature(nullability_on_classes)`.
This is needed for back-compatibility, as previously clang would issue a
hard error when _Nullable appears on a smart pointer.

UBSan's `-fsanitize=nullability` will not check smart-pointer types.
It can be made to do so by synthesizing calls to `operator bool`, but
that's left for future work.
---
 clang/include/clang/Basic/Attr.td  |  3 +-
 clang/include/clang/Basic/AttrDocs.td  | 16 +++
 clang/include/clang/Basic/Features.def |  1 +
 clang/include/clang/Parse/Parser.h |  1 +
 clang/include/clang/Sema/Sema.h|  3 ++
 clang/lib/AST/Type.cpp | 29 +++-
 clang/lib/CodeGen/CGCall.cpp   |  3 +-
 clang/lib/CodeGen/CodeGenFunction.cpp  |  3 +-
 clang/lib/Parse/ParseDeclCXX.cpp   | 33 ++
 clang/lib/Sema/SemaAttr.cpp| 12 +
 clang/lib/Sema/SemaChecking.cpp|  9 
 clang/lib/Sema/SemaDecl.cpp|  4 +-
 clang/lib/Sema/SemaDeclAttr.cpp| 18 
 clang/lib/Sema/SemaInit.cpp|  5 +++
 clang/lib/Sema/SemaOverload.cpp|  7 +++
 clang/lib/Sema/SemaTemplate.cpp|  1 +
 clang/lib/Sema/SemaType.cpp| 18 ++--
 clang/test/SemaCXX/nullability.cpp | 62 +-
 18 files changed, 199 insertions(+), 29 deletions(-)

diff --git a/clang/include/clang/Basic/Attr.td 
b/clang/include/clang/Basic/Attr.td
index fa191c7378dba4..f75aa126cbb939 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2156,9 +2156,10 @@ def TypeNonNull : TypeAttr {
   let Documentation = [TypeNonNullDocs];
 }
 
-def TypeNullable : TypeAttr {
+def TypeNullable : DeclOrTypeAttr {
   let Spellings = [CustomKeyword<"_Nullable">];
   let Documentation = [TypeNullableDocs];
+//  let Subjects = SubjectList<[CXXRecord], ErrorDiag>;
 }
 
 def TypeNullableResult : TypeAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td 
b/clang/include/clang/Basic/AttrDocs.td
index b96fbddd51154c..8c248fb052f19e 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4096,6 +4096,11 @@ non-underscored keywords. For example:
   @property (assign, nullable) NSView *superview;
   @property (readonly, nonnull) NSArray *subviews;
 @end
+
+As well as built-in pointer types, ithe nullability attributes can be attached
+to nullable types from the C++ standard library such as ``std::unique_ptr`` and
+``std::function``, as well as C++ classes marked with the ``_Nullable``
+attribute.
   }];
 }
 
@@ -4130,6 +4135,17 @@ The ``_Nullable`` nullability qualifier indicates that a 
value of the
 int fetch_or_zero(int * _Nullable ptr);
 
 a caller of ``fetch_or_zero`` can provide null.
+
+The ``_Nullable`` attribute on classes indicates that the given class can
+represent null values, and so the ``_Nullable``, ``_Nonnull`` etc qualifiers
+make sense for this type. For example:
+
+  .. code-block:: c
+
+class _Nullable ArenaPointer { ... };
+
+ArenaPointer _Nonnull x = ...;
+ArenaPointer _Nullable y = nullptr;
   }];
 }
 
diff --git a/clang/include/clang/Basic/Features.def 
b/clang/include/clang/Basic/Features.def
index 5fad5fc3623cb6..7c0755b7318306 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -94,6 +94,7 @@ EXTENSION(define_target_os_macros,
 FEATURE(enumerator_attributes, true)
 FEATURE(nullability, true)
 FEATURE(nullability_on_arrays, true)
+FEATURE(nullability_on_classes, true)
 FEATURE(nullability_nullable_result, true)
 FEATURE(memory_sanitizer,
 LangOpts.Sanitize.hasOneOf(SanitizerKind::Memory |
diff --git a/clang/include/clang/Parse/Parser.h 
b/clang/include/clang/Parse/Parser.h
index 071520f535bc95..3d858

[clang] [clang][nullability] allow _Nonnull etc on nullable class types (PR #82705)

[Sam McCall via cfe-commits]

@@ -5955,6 +5955,20 @@ static void handleBuiltinAliasAttr(Sema &S, Decl *D,
   D->addAttr(::new (S.Context) BuiltinAliasAttr(S.Context, AL, Ident));
 }
 
+static void handleNullableTypeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (AL.isUsedAsTypeAttr())
+return;
+
+  if (auto *CRD = dyn_cast(D);
+  !D || !(CRD->isClass() || CRD->isStruct())) {

sam-mccall wrote:

Oops, this was supposed to be `!CRD`! Fixed & will add a testcase.

> Struct as the Subject in Attr.td 

Yeah, unfortunately that doesn't seem to work with attributes that can also 
appear on types :-(

https://github.com/llvm/llvm-project/pull/82705
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][nullability] allow _Nonnull etc on nullable class types (PR #82705)

[Sam McCall via cfe-commits]

@@ -215,6 +215,18 @@ void Sema::inferGslOwnerPointerAttribute(CXXRecordDecl 
*Record) {
   inferGslPointerAttribute(Record, Record);
 }
 
+void Sema::inferNullableClassAttribute(CXXRecordDecl *CRD) {
+  static llvm::StringSet<> Nullable{
+  "auto_ptr", "shared_ptr", "unique_ptr", "exception_ptr",

sam-mccall wrote:

I don't think our nullability concept is a good match for weak pointers - they 
don't behave like other pointers.

If a managed object is shared across threads, then the nullable/non-null 
distinction isn't useful. Allowing it to be used in contracts is probably 
harmful (attractive nuisance & breaks analyses that treat nullable objects 
generically). Since pointers may become null at any point without running any 
code, implementations should always treat them as nullable, generic static 
analysis gives incorrect results, and dynamic checks are racy. Threads are 
involved reasonably often and it's not locally possible to tell.

In the absence of threads, `weak_ptr`s that must not be null should almost 
always be `shared_ptr _Nonnull` instead, and probably are. (This isn't a 
strong objection: `T* _Nonnull` is useful despite `T&` existing - but raw 
pointers are *way* more common than weak ones).

https://github.com/llvm/llvm-project/pull/82705
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits