[PATCH] D38327: [Sema] Put nullability fix-it after the end of the pointer.

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose accepted this revision.
jordan_rose added a comment.
This revision is now accepted and ready to land.

Nice catch, Volodymyr! Looks good to me.


https://reviews.llvm.org/D38327



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

[PATCH] D39035: Unnamed bitfields don't block constant evaluation of constexpr constructors

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose created this revision.
jordan_rose added a project: clang.

C++14 [dcl.constexpr]p4 states that in the body of a constexpr constructor,

> every non-variant non-static data member and base class sub-object shall be 
> initialized

However, [class.bit]p2 notes that

> Unnamed bit-fields are not members and cannot be initialized.

Therefore, we should make sure to filter them out of the check that all fields 
are initialized.

Fixing this makes the constant evaluator a bit smarter, and specifically allows 
constexpr constructors to avoid tripping -Wglobal-constructors when the type 
contains unnamed bitfields.


Repository:
  rL LLVM

https://reviews.llvm.org/D39035

Files:
  lib/AST/ExprConstant.cpp
  test/SemaCXX/constant-expression-cxx11.cpp
  test/SemaCXX/warn-global-constructors.cpp


Index: test/SemaCXX/warn-global-constructors.cpp
===
--- test/SemaCXX/warn-global-constructors.cpp
+++ test/SemaCXX/warn-global-constructors.cpp
@@ -126,3 +126,22 @@
 void *array_storage[1];
 const int &global_reference = *(int *)array_storage;
 }
+
+namespace HasUnnamedBitfield {
+  struct HasUnnamedBitfield {
+unsigned a;
+unsigned : 20;
+unsigned b;
+
+constexpr HasUnnamedBitfield() : a(), b() {}
+constexpr HasUnnamedBitfield(unsigned a, unsigned b) : a(a), b(b) {}
+explicit HasUnnamedBitfield(unsigned a) {}
+  };
+
+  const HasUnnamedBitfield zeroConst{};
+  HasUnnamedBitfield zeroMutable{};
+  const HasUnnamedBitfield explicitConst{1, 2};
+  HasUnnamedBitfield explicitMutable{1, 2};
+  const HasUnnamedBitfield nonConstexprConst{1}; // expected-warning {{global 
constructor}}
+  HasUnnamedBitfield nonConstexprMutable{1}; // expected-warning {{global 
constructor}}
+}
\ No newline at end of file
Index: test/SemaCXX/constant-expression-cxx11.cpp
===
--- test/SemaCXX/constant-expression-cxx11.cpp
+++ test/SemaCXX/constant-expression-cxx11.cpp
@@ -1920,6 +1920,23 @@
 };
 static_assert(X::f(3) == -1, "3 should truncate to -1");
   }
+
+
+  struct HasUnnamedBitfield {
+unsigned a;
+unsigned : 20;
+unsigned b;
+
+constexpr HasUnnamedBitfield() : a(), b() {}
+constexpr HasUnnamedBitfield(unsigned a, unsigned b) : a(a), b(b) {}
+  };
+
+  void testUnnamedBitfield() {
+const HasUnnamedBitfield zero{};
+int a = 1 / zero.b; // expected-error {{division by zero is undefined}}
+const HasUnnamedBitfield oneZero{1, 0};
+int b = 1 / oneZero.b; // expected-error {{division by zero is undefined}}
+  }
 }
 
 namespace ZeroSizeTypes {
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -1783,6 +1783,9 @@
   }
 }
 for (const auto *I : RD->fields()) {
+  if (I->isUnnamedBitfield())
+continue;
+
   if (!CheckConstantExpression(Info, DiagLoc, I->getType(),
Value.getStructField(I->getFieldIndex(
 return false;


Index: test/SemaCXX/warn-global-constructors.cpp
===
--- test/SemaCXX/warn-global-constructors.cpp
+++ test/SemaCXX/warn-global-constructors.cpp
@@ -126,3 +126,22 @@
 void *array_storage[1];
 const int &global_reference = *(int *)array_storage;
 }
+
+namespace HasUnnamedBitfield {
+  struct HasUnnamedBitfield {
+unsigned a;
+unsigned : 20;
+unsigned b;
+
+constexpr HasUnnamedBitfield() : a(), b() {}
+constexpr HasUnnamedBitfield(unsigned a, unsigned b) : a(a), b(b) {}
+explicit HasUnnamedBitfield(unsigned a) {}
+  };
+
+  const HasUnnamedBitfield zeroConst{};
+  HasUnnamedBitfield zeroMutable{};
+  const HasUnnamedBitfield explicitConst{1, 2};
+  HasUnnamedBitfield explicitMutable{1, 2};
+  const HasUnnamedBitfield nonConstexprConst{1}; // expected-warning {{global constructor}}
+  HasUnnamedBitfield nonConstexprMutable{1}; // expected-warning {{global constructor}}
+}
\ No newline at end of file
Index: test/SemaCXX/constant-expression-cxx11.cpp
===
--- test/SemaCXX/constant-expression-cxx11.cpp
+++ test/SemaCXX/constant-expression-cxx11.cpp
@@ -1920,6 +1920,23 @@
 };
 static_assert(X::f(3) == -1, "3 should truncate to -1");
   }
+
+
+  struct HasUnnamedBitfield {
+unsigned a;
+unsigned : 20;
+unsigned b;
+
+constexpr HasUnnamedBitfield() : a(), b() {}
+constexpr HasUnnamedBitfield(unsigned a, unsigned b) : a(a), b(b) {}
+  };
+
+  void testUnnamedBitfield() {
+const HasUnnamedBitfield zero{};
+int a = 1 / zero.b; // expected-error {{division by zero is undefined}}
+const HasUnnamedBitfield oneZero{1, 0};
+int b = 1 / oneZero.b; // expected-error {{division by zero is undefined}}
+  }
 }
 
 namespace ZeroSizeTypes {
Index: lib/AST/ExprConstant.cpp
===

[PATCH] D39035: Unnamed bitfields don't block constant evaluation of constexpr constructors

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

Thanks, Aaron!




Comment at: test/SemaCXX/warn-global-constructors.cpp:130
+
+namespace HasUnnamedBitfield {
+  struct HasUnnamedBitfield {

aaron.ballman wrote:
> Does this namespace require a name? If so, can it be named different than the 
> struct declared within it?
I'm just trying to keep from having my test cases collide with the rest of the 
file and any future additions. I'll change it. (There's no PR or Radar for this 
at the moment; I just came across it while working on something else.)


Repository:
  rL LLVM

https://reviews.llvm.org/D39035



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

[PATCH] D39035: Unnamed bitfields don't block constant evaluation of constexpr constructors

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose closed this revision.
jordan_rose added a comment.

Landed in https://reviews.llvm.org/rL316408.


Repository:
  rL LLVM

https://reviews.llvm.org/D39035



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

[PATCH] D43162: [Parser] (C++) Make -Wextra-semi slightly more useful

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

It's been a long time since that commit, but I think the difference is that 
`-pedantic` / `Extension`-type diagnostics are magic and 
`-Wc++98-compat-pedantic` is not. I like Richard's way better and if it could 
be applied to -Wnewline-eof later as well then that sounds great.


Repository:
  rC Clang

https://reviews.llvm.org/D43162



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

[PATCH] D44498: Sink PrettyDeclStackTrace down to the AST library

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose created this revision.
jordan_rose added reviewers: bruno, rsmith.

...and add some very basic stack trace entries for module building. This would 
have helped track down rdar://problem/38434694 sooner (which I can't share, 
sorry).


Repository:
  rC Clang

https://reviews.llvm.org/D44498

Files:
  include/clang/AST/PrettyDeclStackTrace.h
  include/clang/Sema/PrettyDeclStackTrace.h
  lib/AST/Decl.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseObjc.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp

Index: lib/Serialization/ASTWriterDecl.cpp
===
--- lib/Serialization/ASTWriterDecl.cpp
+++ lib/Serialization/ASTWriterDecl.cpp
@@ -17,6 +17,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclVisitor.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ASTWriter.h"
@@ -2215,6 +2216,9 @@
 }
 
 void ASTWriter::WriteDecl(ASTContext &Context, Decl *D) {
+  PrettyDeclStackTraceEntry CrashInfo(Context, D, SourceLocation(),
+  "serializing");
+
   // Determine the ID for this declaration.
   serialization::DeclID ID;
   assert(!D->isFromASTFile() && "should not be emitting imported decl");
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -18,10 +18,10 @@
 #include "clang/AST/DependentDiagnostic.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
-#include "clang/Sema/PrettyDeclStackTrace.h"
 #include "clang/Sema/Template.h"
 
 using namespace clang;
@@ -3912,7 +3912,7 @@
   InstantiatingTemplate Inst(*this, PointOfInstantiation, Function);
   if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
 return;
-  PrettyDeclStackTraceEntry CrashInfo(*this, Function, SourceLocation(),
+  PrettyDeclStackTraceEntry CrashInfo(Context, Function, SourceLocation(),
   "instantiating function definition");
 
   // The instantiation is visible here, even if it was first declared in an
@@ -4321,7 +4321,7 @@
   InstantiatingTemplate Inst(*this, PointOfInstantiation, Var);
   if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
 return;
-  PrettyDeclStackTraceEntry CrashInfo(*this, Var, SourceLocation(),
+  PrettyDeclStackTraceEntry CrashInfo(Context, Var, SourceLocation(),
   "instantiating variable initializer");
 
   // The instantiation is visible here, even if it was first declared in an
@@ -4434,7 +4434,7 @@
   InstantiatingTemplate Inst(*this, PointOfInstantiation, Var);
   if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
 return;
-  PrettyDeclStackTraceEntry CrashInfo(*this, Var, SourceLocation(),
+  PrettyDeclStackTraceEntry CrashInfo(Context, Var, SourceLocation(),
   "instantiating variable definition");
 
   // If we're performing recursive template instantiation, create our own
@@ -5238,7 +5238,7 @@
   break;
 }
 
-PrettyDeclStackTraceEntry CrashInfo(*this, Var, SourceLocation(),
+PrettyDeclStackTraceEntry CrashInfo(Context, Var, SourceLocation(),
 "instantiating variable definition");
 bool DefinitionRequired = Var->getTemplateSpecializationKind() ==
   TSK_ExplicitInstantiationDefinition;
Index: lib/Sema/SemaTemplateInstantiate.cpp
===
--- lib/Sema/SemaTemplateInstantiate.cpp
+++ lib/Sema/SemaTemplateInstantiate.cpp
@@ -18,11 +18,11 @@
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
-#include "clang/Sema/PrettyDeclStackTrace.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
 
@@ -2011,7 +2011,7 @@
   if (Inst.isInvalid())
 return true;
   assert(!Inst.isAlreadyInstantiating() && "should have been caught by caller");
-  PrettyDeclStackTraceEntry CrashInfo(*this, Instantiation, SourceLocation(),
+  PrettyDeclStackTraceEntry CrashInfo(Context, Instantiation, SourceLocation(),
   "instantiating class definition");
 
   // Enter the scope of this instan

[PATCH] D57076: [ObjC generics] Fix applying `__kindof` to the type parameter.

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

I //think// this is reasonable but Doug was the one who worked on this. I 
wonder if it also helps with the test cases in rdar://problem/24619481.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57076/new/

https://reviews.llvm.org/D57076



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

[PATCH] D59197: [NFC][clang][PCH][ObjC] Add some missing `VisitStmt(S); `

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose accepted this revision.
jordan_rose added a comment.
This revision is now accepted and ready to land.

I'm no Clang serialization expert but this makes sense to me. It's consistent 
with all the other statement visitor methods.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59197/new/

https://reviews.llvm.org/D59197



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

[PATCH] D59176: Modules: Add LangOptions::CacheGeneratedPCH

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

This commit by itself doesn't change any behavior, right?




Comment at: clang/lib/Frontend/FrontendActions.cpp:115
   CI.getPreprocessorOpts().AllowPCHWithCompilerErrors,
-  FrontendOpts.IncludeTimestamps));
+  FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(

What's the `+` for?



Comment at: clang/lib/Frontend/FrontendActions.cpp:182
+  CI.getFrontendOpts().BuildingImplicitModule &&
+  CI.getLangOpts().isCompilingModule()));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(

Why is this the condition, as opposed to just "do this for all modules, don't 
do it for PCHs"? And doesn't `BuildingImplicitModule` imply 
`isCompilingModule()`? (Note that `BuildingImplicitModule` probably isn't the 
same as the original condition `ImplicitModules`.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59176/new/

https://reviews.llvm.org/D59176



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

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: lib/CodeGen/CGObjCMac.cpp:7285
 
-Entry = new llvm::GlobalVariable(CGM.getModule(), 
ObjCTypes.ClassnfABIPtrTy,
+Entry = new llvm::GlobalVariable(CGM.getModule(), ClassGV->getType(),
  false, llvm::GlobalValue::PrivateLinkage,

Is there a concern here in the non-stub case if GetClassGlobal no longer 
produces a ObjCTypes.ClassnfABIPtrTy? (Probably not, but thought I'd check 
[again].)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59628/new/

https://reviews.llvm.org/D59628



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

[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 🔍

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

I don't think there's ever a reason to call `[super self]`, and doing so 
through a macro could easily indicate a bug.

Diagnostic nitpick: the Objective-C term is "init method", not "initializer".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59806/new/

https://reviews.llvm.org/D59806



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

[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 🔍

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

Ooh, I should have remembered "designated initializer". I guess it doesn't 
matter so much either way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59806/new/

https://reviews.llvm.org/D59806



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

[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+  internal::Matcher,

aaron.ballman wrote:
> stephanemoore wrote:
> > aaron.ballman wrote:
> > > stephanemoore wrote:
> > > > I am still uncertain about the naming.
> > > > 
> > > > `isSubclassOf` seemed too generic as that could apply to C++ classes.
> > > > `objcIsSubclassOf` seemed unconventional as a matcher name.
> > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed 
> > > > awkwardly lengthy.
> > > > Creating a new namespace `clang::ast_matchers::objc` seemed 
> > > > unprecedented.
> > > > 
> > > > I am happy to change the name if you think another name would be more 
> > > > appropriate.
> > > Does ObjC use the term "derived" by any chance? We already have 
> > > `isDerivedFrom`, so I'm wondering if we can use that to also match on an 
> > > `ObjCInterfaceDecl`?
> > Objective-C doesn't generally use the term "derived" (for example, see 
> > archive of [Programming With Objective-C > Defining 
> > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)).
> >  With that said, I don't think it's unreasonable or incorrect to use the 
> > term "derived" to describe inheritance in Objective-C. The behavior of this 
> > matcher is also consistent with the behavior of `isDerivedFrom`. In order 
> > to change `isDerivedFrom`, I would also need to update 
> > `isSameOrDerivedFrom`. That would probably be a good thing so that 
> > derivation matching feature set is consistent for C++ and Objective-C 
> > language variants.
> > 
> > Let me take a crack at merging this into `isDerivedFrom`.
> I agree that if we go with `isDerivedFrom`, you should update 
> `isSameOrDerivedFrom` at the same time.
`isSubclassOf` sounds right to me, and since ObjC and C++ class hierarchies 
can't mix, it _should_ be okay, right? They're analogous concepts.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60543/new/

https://reviews.llvm.org/D60543



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

[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+  internal::Matcher,

aaron.ballman wrote:
> jordan_rose wrote:
> > aaron.ballman wrote:
> > > stephanemoore wrote:
> > > > aaron.ballman wrote:
> > > > > stephanemoore wrote:
> > > > > > I am still uncertain about the naming.
> > > > > > 
> > > > > > `isSubclassOf` seemed too generic as that could apply to C++ 
> > > > > > classes.
> > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name.
> > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed 
> > > > > > awkwardly lengthy.
> > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed 
> > > > > > unprecedented.
> > > > > > 
> > > > > > I am happy to change the name if you think another name would be 
> > > > > > more appropriate.
> > > > > Does ObjC use the term "derived" by any chance? We already have 
> > > > > `isDerivedFrom`, so I'm wondering if we can use that to also match on 
> > > > > an `ObjCInterfaceDecl`?
> > > > Objective-C doesn't generally use the term "derived" (for example, see 
> > > > archive of [Programming With Objective-C > Defining 
> > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)).
> > > >  With that said, I don't think it's unreasonable or incorrect to use 
> > > > the term "derived" to describe inheritance in Objective-C. The behavior 
> > > > of this matcher is also consistent with the behavior of 
> > > > `isDerivedFrom`. In order to change `isDerivedFrom`, I would also need 
> > > > to update `isSameOrDerivedFrom`. That would probably be a good thing so 
> > > > that derivation matching feature set is consistent for C++ and 
> > > > Objective-C language variants.
> > > > 
> > > > Let me take a crack at merging this into `isDerivedFrom`.
> > > I agree that if we go with `isDerivedFrom`, you should update 
> > > `isSameOrDerivedFrom` at the same time.
> > `isSubclassOf` sounds right to me, and since ObjC and C++ class hierarchies 
> > can't mix, it _should_ be okay, right? They're analogous concepts.
> > isSubclassOf sounds right to me, and since ObjC and C++ class hierarchies 
> > can't mix, it _should_ be okay, right? They're analogous concepts.
> 
> In the AST matchers, we try to overload the matchers that have similar 
> behavior. My concern is that a user will reach for `isSubclassOf()` when they 
> really mean `isDerivedFrom()` or vice versa, and only through annoying error 
> messages learns about their mistake. Given that we already have 
> `isDerivedFrom()` and renaming it would break code, I was trying to determine 
> whether using that name for both C++ derivation and ObjC derivation would be 
> acceptable.
Ah, I see what you mean. Yes, I guess it's more important to be consistent than 
to perfectly match the terminology. You will certainly confuse an ObjC-only 
developer at first by using "non-standard" terminology, but any developer has 
to learn a certain amount of compiler-isms anyway using AST matchers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60543/new/

https://reviews.llvm.org/D60543



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

[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+  internal::Matcher,

aaron.ballman wrote:
> jordan_rose wrote:
> > aaron.ballman wrote:
> > > jordan_rose wrote:
> > > > aaron.ballman wrote:
> > > > > stephanemoore wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > stephanemoore wrote:
> > > > > > > > I am still uncertain about the naming.
> > > > > > > > 
> > > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ 
> > > > > > > > classes.
> > > > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name.
> > > > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed 
> > > > > > > > awkwardly lengthy.
> > > > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed 
> > > > > > > > unprecedented.
> > > > > > > > 
> > > > > > > > I am happy to change the name if you think another name would 
> > > > > > > > be more appropriate.
> > > > > > > Does ObjC use the term "derived" by any chance? We already have 
> > > > > > > `isDerivedFrom`, so I'm wondering if we can use that to also 
> > > > > > > match on an `ObjCInterfaceDecl`?
> > > > > > Objective-C doesn't generally use the term "derived" (for example, 
> > > > > > see archive of [Programming With Objective-C > Defining 
> > > > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)).
> > > > > >  With that said, I don't think it's unreasonable or incorrect to 
> > > > > > use the term "derived" to describe inheritance in Objective-C. The 
> > > > > > behavior of this matcher is also consistent with the behavior of 
> > > > > > `isDerivedFrom`. In order to change `isDerivedFrom`, I would also 
> > > > > > need to update `isSameOrDerivedFrom`. That would probably be a good 
> > > > > > thing so that derivation matching feature set is consistent for C++ 
> > > > > > and Objective-C language variants.
> > > > > > 
> > > > > > Let me take a crack at merging this into `isDerivedFrom`.
> > > > > I agree that if we go with `isDerivedFrom`, you should update 
> > > > > `isSameOrDerivedFrom` at the same time.
> > > > `isSubclassOf` sounds right to me, and since ObjC and C++ class 
> > > > hierarchies can't mix, it _should_ be okay, right? They're analogous 
> > > > concepts.
> > > > isSubclassOf sounds right to me, and since ObjC and C++ class 
> > > > hierarchies can't mix, it _should_ be okay, right? They're analogous 
> > > > concepts.
> > > 
> > > In the AST matchers, we try to overload the matchers that have similar 
> > > behavior. My concern is that a user will reach for `isSubclassOf()` when 
> > > they really mean `isDerivedFrom()` or vice versa, and only through 
> > > annoying error messages learns about their mistake. Given that we already 
> > > have `isDerivedFrom()` and renaming it would break code, I was trying to 
> > > determine whether using that name for both C++ derivation and ObjC 
> > > derivation would be acceptable.
> > Ah, I see what you mean. Yes, I guess it's more important to be consistent 
> > than to perfectly match the terminology. You will certainly confuse an 
> > ObjC-only developer at first by using "non-standard" terminology, but any 
> > developer has to learn a certain amount of compiler-isms anyway using AST 
> > matchers.
> Okay, so it sounds like it wouldn't be hugely problematic to go with 
> `isDerivedFrom()`, just that it may sound a bit confusing at first to an ObjC 
> developer. Are there some words you think we should add to the documentation 
> to help an ObjC developer searching for this functionality?
I think just including "subclass" is sufficient. For example, the current doc 
comment is

```
/// Matches C++ classes that are directly or indirectly derived from
/// a class matching \c Base.
```

and it could be changed to something like

```
/// Matches C++ classes that are directly or indirectly derived from
/// a class matching \c Base, or Objective-C classes that directly or
/// indirectly subclass a class matching \c Base.
```

A little clunky, but you get what I mean.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60543/new/

https://reviews.llvm.org/D60543



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

[PATCH] D65212: [analyzer] Fix exporting SARIF files from scan-build on Windows

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

Oops. It looks like there's another place where this pattern shows up (see 
rC139382 ). The other one should probably be 
changed as well.

(Thanks for diverging from test(1), perl…)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65212/new/

https://reviews.llvm.org/D65212



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:16379
+   !getLangOpts().CPlusPlus && !FD->hasAttr() &&
+   !FD->getType()->getAs()) {
+  // For backward compatibility, fields of C unions declared in system

This check seems a little worrisome since we could have some other kind of 
attributed type. Is there an easy way to check specifically for a lifetime 
qualifier?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65256/new/

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

In D65256#1601410 , @rjmccall wrote:

> These were unavailable in system headers before because otherwise we would've 
> had to make them invalid.  Since these unions are no longer otherwise 
> invalid, there shouldn't be a problem with allowing them in system headers, 
> and in fact making the semantics vary that way seems quite problematic.  Now, 
> specific *uses* in system headers might still appear to be invalid — e.g. an 
> ObjC ivar of type `union { __strong id x; }` — and the right behavior is 
> definitely that those use sites should be marked as invalid instead of 
> refusing to compile the system header.


That's the right answer on paper, but it's source-breaking in practice, for 
both Swift and Objective-C. On the Objective-C side, someone could have been 
using, say, `glob_t` in a copyable way in their ARC code, never touching the 
block field, and it would have been working fine. On the Swift side, we won't 
be able to import such a union at all when previously we would have.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65256/new/

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

I'm personally still of the opinion that allowing non-trivial fields in unions 
was a mistake, but it's too late to change that as well.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65256/new/

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

In D65256#1601509 , @rjmccall wrote:

> Sorry, am I missing something?  Such a union would've been either ill-formed 
> or unavailable in ARC (depending on where it was declared) before this recent 
> work.


Apparently that was not the case if it was in a system header. Instead, Clang 
marked the //member// unavailable rather than the entire union.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65256/new/

https://reviews.llvm.org/D65256



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

[PATCH] D63943: ObjC: Squeeze in one more bit for the count of protocols in 'id' types

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose created this revision.
jordan_rose added reviewers: jfb, doug.gregor, dexonsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Someone actually had an Objective-C object pointer type with more than 64 
protocols, probably collected through typedefs. Raise the ceiling by one order 
of magnitude...even though this is just kicking the can down the road, and we 
need a proper diagnostic for this.


Repository:
  rC Clang

https://reviews.llvm.org/D63943

Files:
  include/clang/AST/Type.h
  lib/AST/Type.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -621,7 +621,7 @@
Base->isVariablyModifiedType(),
Base->containsUnexpandedParameterPack()),
   BaseType(Base) {
-  ObjCObjectTypeBits.IsKindOf = isKindOf;
+  CachedSuperClassAndIsKindOf.setInt(isKindOf);
 
   ObjCObjectTypeBits.NumTypeArgs = typeArgs.size();
   assert(getTypeArgsAsWritten().size() == typeArgs.size() &&
@@ -1454,20 +1454,20 @@
   // superclass type.
   ObjCInterfaceDecl *classDecl = getInterface();
   if (!classDecl) {
-CachedSuperClassType.setInt(true);
+CachedSuperClassAndIsKindOf.setPointer({nullptr, true});
 return;
   }
 
   // Extract the superclass type.
   const ObjCObjectType *superClassObjTy = classDecl->getSuperClassType();
   if (!superClassObjTy) {
-CachedSuperClassType.setInt(true);
+CachedSuperClassAndIsKindOf.setPointer({nullptr, true});
 return;
   }
 
   ObjCInterfaceDecl *superClassDecl = superClassObjTy->getInterface();
   if (!superClassDecl) {
-CachedSuperClassType.setInt(true);
+CachedSuperClassAndIsKindOf.setPointer({nullptr, true});
 return;
   }
 
@@ -1476,14 +1476,14 @@
   QualType superClassType(superClassObjTy, 0);
   ObjCTypeParamList *superClassTypeParams = superClassDecl->getTypeParamList();
   if (!superClassTypeParams) {
-CachedSuperClassType.setPointerAndInt(
-  superClassType->castAs(), true);
+CachedSuperClassAndIsKindOf.setPointer({
+  superClassType->castAs(), true});
 return;
   }
 
   // If the superclass reference is unspecialized, return it.
   if (superClassObjTy->isUnspecialized()) {
-CachedSuperClassType.setPointerAndInt(superClassObjTy, true);
+CachedSuperClassAndIsKindOf.setPointer({superClassObjTy, true});
 return;
   }
 
@@ -1491,8 +1491,8 @@
   // parameters in the superclass reference to substitute.
   ObjCTypeParamList *typeParams = classDecl->getTypeParamList();
   if (!typeParams) {
-CachedSuperClassType.setPointerAndInt(
-  superClassType->castAs(), true);
+CachedSuperClassAndIsKindOf.setPointer({
+  superClassType->castAs(), true});
 return;
   }
 
@@ -1502,20 +1502,19 @@
 QualType unspecializedSuper
   = classDecl->getASTContext().getObjCInterfaceType(
   superClassObjTy->getInterface());
-CachedSuperClassType.setPointerAndInt(
-  unspecializedSuper->castAs(),
-  true);
+CachedSuperClassAndIsKindOf.setPointer({
+  unspecializedSuper->castAs(), true});
 return;
   }
 
   // Substitute the provided type arguments into the superclass type.
   ArrayRef typeArgs = getTypeArgs();
   assert(typeArgs.size() == typeParams->size());
-  CachedSuperClassType.setPointerAndInt(
+  CachedSuperClassAndIsKindOf.setPointer({
 superClassType.substObjCTypeArgs(classDecl->getASTContext(), typeArgs,
  ObjCSubstitutionContext::Superclass)
   ->castAs(),
-true);
+true});
 }
 
 const ObjCInterfaceType *ObjCObjectPointerType::getInterfaceType() const {
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -1556,10 +1556,7 @@
 unsigned NumTypeArgs : 7;
 
 /// The number of protocols stored directly on this object type.
-unsigned NumProtocols : 6;
-
-/// Whether this is a "kindof" type.
-unsigned IsKindOf : 1;
+unsigned NumProtocols : 7;
   };
 
   class ReferenceTypeBitfields {
@@ -5560,9 +5557,12 @@
   /// Either a BuiltinType or an InterfaceType or sugar for either.
   QualType BaseType;
 
-  /// Cached superclass type.
-  mutable llvm::PointerIntPair
-CachedSuperClassType;
+  using CachedSuperClassStorage =
+  llvm::PointerIntPair;
+
+  /// Cached superclass type plus whether this type was written with '__kindof'.
+  mutable llvm::PointerIntPair
+  CachedSuperClassAndIsKindOf;
 
   QualType *getTypeArgStorage();
   const QualType *getTypeArgStorage() const {
@@ -5592,7 +5592,6 @@
   BaseType(QualType(this_(), 0)) {
 ObjCObjectTypeBits.NumProtocols = 0;
 ObjCObjectTypeBits.NumTypeArgs = 0;
-ObjCObjectTypeBits.IsKindOf = 0;
   }
 
   void computeSuperClassTypeSlow() const;
@@ -5658,7 +5657,9 @@
   }
 
   /// Whether this is a "__kindof" type as written.
-  bool isKindOfTypeAsWritten() const { return ObjCObjectTypeBits.IsKind

[PATCH] D63943: ObjC: Squeeze in one more bit for the count of protocols in 'id' types

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

Yeah, I'll write one.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63943/new/

https://reviews.llvm.org/D63943



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

[PATCH] D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

I think Aaron and John have covered any comments I would make! I'm glad this 
came out so simply, though. I was afraid it'd be a much larger change than just 
expanding what was already there.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55865/new/

https://reviews.llvm.org/D55865



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

[PATCH] D68430: Don't use object libraries with Xcode

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose created this revision.
jordan_rose added a reviewer: beanz.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.

Undoes some of the effects of D61909  when 
using the Xcode CMake generator—it doesn't handle object libraries correctly at 
all. Attempts to still honor `BUILD_SHARED_LIBS` for Xcode, but I didn't 
actually test it. Should have no effect on non-Xcode generators.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68430

Files:
  clang/cmake/modules/AddClang.cmake


Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -86,11 +86,15 @@
 # llvm_add_library ignores BUILD_SHARED_LIBS if STATIC is explicitly set,
 # so we need to handle it here.
 if(BUILD_SHARED_LIBS)
-  set(LIBTYPE SHARED OBJECT)
+  set(LIBTYPE SHARED)
 else()
-  set(LIBTYPE STATIC OBJECT)
+  set(LIBTYPE STATIC)
+endif()
+if(NOT XCODE)
+  # The Xcode generator doesn't handle object libraries correctly.
+  list(APPEND LIBTYPE OBJECT)
+  set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
 endif()
-set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
   llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 


Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -86,11 +86,15 @@
 # llvm_add_library ignores BUILD_SHARED_LIBS if STATIC is explicitly set,
 # so we need to handle it here.
 if(BUILD_SHARED_LIBS)
-  set(LIBTYPE SHARED OBJECT)
+  set(LIBTYPE SHARED)
 else()
-  set(LIBTYPE STATIC OBJECT)
+  set(LIBTYPE STATIC)
+endif()
+if(NOT XCODE)
+  # The Xcode generator doesn't handle object libraries correctly.
+  list(APPEND LIBTYPE OBJECT)
+  set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
 endif()
-set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
   llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68430: Don't use object libraries with Xcode

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

I'm not quite sure //what// it's doing. The executable targets end up trying to 
link against the static libraries anyway, which of course haven't been built. 
It's possible that this is because the LIBTYPE is both STATIC and OBJECT and if 
it were just OBJECT we might be better off, but I'm not sure if Xcode's IDE 
features will be happy with a target that doesn't actually produce a library. I 
can try it if you want, though.

(I did look at CMake's Xcode generator logic for OBJECT targets and it looks 
completely bonkers to me, as if it's still building the static library but then 
removing it to make sure it's not depended on or something. Even if it builds 
cleanly I don't trust it to do dependency analysis correctly.)

> This has the side-effect of making `libclang_cpp` effectively empty when you 
> build with Xcode.

I can remove that target if you want, or have it link the libraries normally.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68430/new/

https://reviews.llvm.org/D68430



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

[PATCH] D22391: [Sema] Add warning for implicitly casting a null constant to a non null pointer type

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a subscriber: dergachev.a.
jordan_rose added a comment.

Sorry for the delay. I think this is mostly good, but I do still have a concern 
about the diagnostics change.




Comment at: lib/Sema/SemaExpr.cpp:7117
+if (E && S.checkNonNullExpr(E))
+  return NullabilityKind::Nullable;
+

ahatanak wrote:
> jordan_rose wrote:
> > This isn't quite correct, unfortunately. `(_Nonnull id)nil` should be 
> > considered non-nullable, since it's the canonical way to avoid all these 
> > warnings. It might just be good enough to move this check below the 
> > `getNullability` one, though.
> Sema::CheckNonNullExpr checks the nullability of the type of the expression 
> first and returns false if there is a cast to `_Nonnull`.
Hm, okay, I see you have a test. Sorry for the noise.



Comment at: lib/Sema/SemaExpr.cpp:7162
+  bool IsBin, Expr *LHSExpr,
+  Expr *RHSExpr, ASTContext &Ctx) {
   if (!ResTy->isAnyPointerType())

Nitpick: you could use `const` on the Exprs here.



Comment at: lib/Sema/SemaExpr.cpp:11103
 
+  if (const auto *DeclRef = dyn_cast(LHSExpr))
+checkNullConstantToNonNull(DeclRef->getType(), RHS.get());

This could come later, but what about struct members or ObjC properties or ObjC 
subscripts? Seems like you could just check the type of the LHS.



Comment at: test/Analysis/nullability_nullonly.mm:103
 void testObjCARCExplicitZeroInitialization() {
-  TestObject * _Nonnull explicitlyZeroInitialized = nil; // expected-warning 
{{nil assigned to a pointer which is expected to have non-null value}}
+  TestObject * _Nonnull explicitlyZeroInitialized = nil; // expected-warning 
{{nil assigned to a pointer which is expected to have non-null value}} 
expected-warning{{implicitly casting a null constant to non-nullable pointer 
type 'TestObject * _Nonnull __strong'}}
 }

@dergachev.a, what do you think here? Is it okay that the analyzer diagnoses 
this in addition to the new warning?



Comment at: test/Sema/conditional-expr.c:20
   vp = 0 ? (double *)0 : (void *)0;
-  ip = 0 ? (double *)0 : (void *)0; // expected-warning {{incompatible pointer 
types assigning to 'int *' from 'double *'}}
+  ip = 0 ? (double *)0 : (void *)0; // expected-warning {{incompatible pointer 
types assigning to 'int *' from 'double * _Nullable'}}
 

This seems like an unfortunate change to make, since most people do not bother 
with nullability.


Repository:
  rC Clang

https://reviews.llvm.org/D22391



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

[PATCH] D22391: [Sema] Add warning for implicitly casting a null constant to a non null pointer type

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: test/Sema/conditional-expr.c:20
   vp = 0 ? (double *)0 : (void *)0;
-  ip = 0 ? (double *)0 : (void *)0; // expected-warning {{incompatible pointer 
types assigning to 'int *' from 'double *'}}
+  ip = 0 ? (double *)0 : (void *)0; // expected-warning {{incompatible pointer 
types assigning to 'int *' from 'double * _Nullable'}}
 

ahatanak wrote:
> ahatanak wrote:
> > jordan_rose wrote:
> > > This seems like an unfortunate change to make, since most people do not 
> > > bother with nullability.
> > Yes, this is unfortunate, but I'm not sure what's the right way to avoid 
> > printing nullability specifiers in the diagnostic message. Do you have any 
> > suggestions?
> It looks like I can use PrintingPolicy to print the nullability specifier 
> only when needed.
> 
> I think it's also possible to add a flag to Expr that indicates the Expr is 
> possibly null. For example, when an Expr is an IntegerLiteral of value 0, or 
> a CastExpr or a ConditionalOperator has a subexpression whose flag is set. 
> This could be a better solution than the current solution in this patch.
Whew. I hadn't had the chance to look at PrintingPolicy and was afraid you'd 
have to add a new flag to diagnostics or something to specify whether 
nullability was relevant.

An additional flag on Expr seems like overkill to me, given that 
`isNullPointerConstant` already exists. But I don't work in Clang these days, 
so maybe I'm wrong and it is something worth caching. 


Repository:
  rC Clang

https://reviews.llvm.org/D22391



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

[PATCH] D68430: Don't use object libraries with Xcode

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

In D68430#1693965 , @beanz wrote:

> clang_cpp can't link the libraries "normally" because it has no unresolved 
> symbols to force the contents of the libraries to link. I don't like it, but 
> I think the best option is to disable clang_cpp under Xcode. You can add `AND 
> XCODE` to the `if` on line 2 of clang/tools/clang-shlib/CMakeLists.txt, and 
> that should do the trick.


I do think it's okay to just not support that target for Xcode, but another 
option would be to add `-all_load` to the linker line, the ld64 option that 
just unconditionally includes all archives. What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68430/new/

https://reviews.llvm.org/D68430



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

[PATCH] D68430: Don't use object libraries with Xcode

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose updated this revision to Diff 223250.
jordan_rose added a comment.

Okay, having Xcode force-load the static libraries doesn't seem bad at all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68430/new/

https://reviews.llvm.org/D68430

Files:
  clang/cmake/modules/AddClang.cmake
  clang/tools/clang-shlib/CMakeLists.txt


Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- clang/tools/clang-shlib/CMakeLists.txt
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -6,7 +6,13 @@
 get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
 
 foreach (lib ${clang_libs})
-  list(APPEND _OBJECTS $)
+  if(XCODE)
+# Xcode doesn't support object libraries, so we have to trick it into
+# linking the static libraries instead.
+list(APPEND _DEPS "-force_load" ${lib})
+  else()
+list(APPEND _OBJECTS $)
+  endif()
   list(APPEND _DEPS $)
   list(APPEND _DEPS $)
 endforeach ()
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -86,9 +86,13 @@
 # llvm_add_library ignores BUILD_SHARED_LIBS if STATIC is explicitly set,
 # so we need to handle it here.
 if(BUILD_SHARED_LIBS)
-  set(LIBTYPE SHARED OBJECT)
+  set(LIBTYPE SHARED)
 else()
-  set(LIBTYPE STATIC OBJECT)
+  set(LIBTYPE STATIC)
+endif()
+if(NOT XCODE)
+  # The Xcode generator doesn't handle object libraries correctly.
+  list(APPEND LIBTYPE OBJECT)
 endif()
 set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()


Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- clang/tools/clang-shlib/CMakeLists.txt
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -6,7 +6,13 @@
 get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
 
 foreach (lib ${clang_libs})
-  list(APPEND _OBJECTS $)
+  if(XCODE)
+# Xcode doesn't support object libraries, so we have to trick it into
+# linking the static libraries instead.
+list(APPEND _DEPS "-force_load" ${lib})
+  else()
+list(APPEND _OBJECTS $)
+  endif()
   list(APPEND _DEPS $)
   list(APPEND _DEPS $)
 endforeach ()
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -86,9 +86,13 @@
 # llvm_add_library ignores BUILD_SHARED_LIBS if STATIC is explicitly set,
 # so we need to handle it here.
 if(BUILD_SHARED_LIBS)
-  set(LIBTYPE SHARED OBJECT)
+  set(LIBTYPE SHARED)
 else()
-  set(LIBTYPE STATIC OBJECT)
+  set(LIBTYPE STATIC)
+endif()
+if(NOT XCODE)
+  # The Xcode generator doesn't handle object libraries correctly.
+  list(APPEND LIBTYPE OBJECT)
 endif()
 set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68430: Don't use object libraries with Xcode

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose closed this revision.
jordan_rose added a comment.

Committed as rC373769 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68430/new/

https://reviews.llvm.org/D68430



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

[PATCH] D68846: Do the bare minimum to get ClangdXPC.framework building with CMake's Xcode generator

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose created this revision.
jordan_rose added reviewers: beanz, jkorous.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, 
dexonsmith, MaskRay, ilya-biryukov, mgorny.
Herald added a project: clang.

The output directories for CMake's Xcode project generator are specific to the 
configuration, and so looking in `CMAKE_LIBRARY_OUTPUT_DIRECTORY` isn't going 
to work. Fortunately, CMake already provides generator expressions to find the 
output of a given target.

I call this the bare minimum because the //built// framework isn't going to 
respect the configuration; that is, I can't have both Debug and RelWithDebInfo 
variants of ClangdXPC.framework at the same time like I can with normal library 
or executable targets. To do that we'd have to put the framework in a 
configuration-specific output directory too…or use CMake's native support for 
frameworks instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D68846

Files:
  clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake


Index: clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
===
--- clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
+++ clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
@@ -28,7 +28,7 @@
 
 # Copy the framework binary.
 COMMAND ${CMAKE_COMMAND} -E copy
-   "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/lib${target}.dylib"
+   "$"
"${CLANGD_FRAMEWORK_OUT_LOCATION}/${name}"
 
 # Copy the XPC Service PLIST.
@@ -38,7 +38,7 @@
 
 # Copy the Clangd binary.
 COMMAND ${CMAKE_COMMAND} -E copy
-  "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/clangd"
+  "$"
   "${CLANGD_XPC_SERVICE_OUT_LOCATION}/MacOS/clangd"
 
  COMMAND ${CMAKE_COMMAND} -E create_symlink "A"


Index: clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
===
--- clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
+++ clang-tools-extra/clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
@@ -28,7 +28,7 @@
 
 # Copy the framework binary.
 COMMAND ${CMAKE_COMMAND} -E copy
-   "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/lib${target}.dylib"
+   "$"
"${CLANGD_FRAMEWORK_OUT_LOCATION}/${name}"
 
 # Copy the XPC Service PLIST.
@@ -38,7 +38,7 @@
 
 # Copy the Clangd binary.
 COMMAND ${CMAKE_COMMAND} -E copy
-  "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/clangd"
+  "$"
   "${CLANGD_XPC_SERVICE_OUT_LOCATION}/MacOS/clangd"
 
  COMMAND ${CMAKE_COMMAND} -E create_symlink "A"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68846: Do the bare minimum to get ClangdXPC.framework building with CMake's Xcode generator

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose closed this revision.
jordan_rose added a comment.

Committed in rCTE374494 .


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68846/new/

https://reviews.llvm.org/D68846



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

[PATCH] D85287: Extend -Wtautological-bitwise-compare "bitwise or with non-zero value" warnings

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

Nothing seems too mysterious here, but why limit to `constexpr` instead of 
anything where Clang can see a constant value, which would make it work in C as 
well? (If you're allowing `constexpr`, you're already allowing arbitrary work.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85287/new/

https://reviews.llvm.org/D85287

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

[PATCH] D44498: Sink PrettyDeclStackTrace down to the AST library

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose closed this revision.
jordan_rose added a comment.

Committed in https://reviews.llvm.org/rL328276.


Repository:
  rC Clang

https://reviews.llvm.org/D44498



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

[PATCH] D27837: Add fix-it notes to the nullability consistency warning

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose created this revision.
jordan_rose added a reviewer: doug.gregor.
jordan_rose added a subscriber: cfe-commits.
jordan_rose set the repository for this revision to rL LLVM.

This is especially important for arrays, since no one knows the proper syntax 
for putting qualifiers in arrays.

  nullability.h:3:26: warning: array parameter is missing a nullability type 
specifier (_Nonnull, _Nullable, or _Null_unspecified)
  void arrayParameter(int x[]);
   ^
  nullability.h:3:26: note: insert '_Nullable' if the array parameter may be 
null
  void arrayParameter(int x[]);
   ^
_Nullable
  nullability.h:3:26: note: insert '_Nonnull' if the array parameter should 
never be null
  void arrayParameter(int x[]);
   ^
_Nonnull

rdar://problem/29524992


Repository:
  rL LLVM

https://reviews.llvm.org/D27837

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaType.cpp
  test/FixIt/Inputs/nullability.h
  test/FixIt/nullability.mm
  test/SemaObjCXX/Inputs/nullability-consistency-1.h
  test/SemaObjCXX/Inputs/nullability-consistency-2.h
  test/SemaObjCXX/Inputs/nullability-consistency-3.h
  test/SemaObjCXX/Inputs/nullability-consistency-4.h
  test/SemaObjCXX/Inputs/nullability-consistency-5.h
  test/SemaObjCXX/Inputs/nullability-consistency-6.h
  test/SemaObjCXX/Inputs/nullability-consistency-7.h
  test/SemaObjCXX/Inputs/nullability-consistency-8.h
  test/SemaObjCXX/Inputs/nullability-consistency-arrays.h
  
test/SemaObjCXX/Inputs/nullability-consistency-system/nullability-consistency-system.h
  test/SemaObjCXX/Inputs/nullability-pragmas-1.h

Index: test/SemaObjCXX/Inputs/nullability-pragmas-1.h
===
--- test/SemaObjCXX/Inputs/nullability-pragmas-1.h
+++ test/SemaObjCXX/Inputs/nullability-pragmas-1.h
@@ -9,6 +9,8 @@
 struct X { };
 
 void f1(int *x); // expected-warning{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2{{insert '_Nonnull' if the pointer should never be null}}
 
 typedef struct __attribute__((objc_bridge(NSError))) __CFError *CFErrorRef;
 typedef NSError *NSErrorPtr;
@@ -39,15 +41,29 @@
 A * _Null_unspecified f16(void);
 void f17(CFErrorRef *error); // expected-note{{no known conversion from 'A * _Nonnull' to 'CFErrorRef  _Nullable * _Nullable' (aka '__CFError **') for 1st argument}}
 void f18(A **); // expected-warning 2{{pointer is missing a nullability type specifier}}
+// expected-note@-1 2 {{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2 2 {{insert '_Nonnull' if the pointer should never be null}}
 void f19(CFErrorRefPtr error); // expected-warning{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2{{insert '_Nonnull' if the pointer should never be null}}
 
 void g1(int (^)(int, int));
 void g2(int (^ *bp)(int, int)); // expected-warning{{block pointer is missing a nullability type specifier}}
-// expected-warning@-1{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if the block pointer may be null}}
+// expected-note@-2{{insert '_Nonnull' if the block pointer should never be null}}
+// expected-warning@-3{{pointer is missing a nullability type specifier}}
+// expected-note@-4{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-5{{insert '_Nonnull' if the pointer should never be null}}
 void g3(block_ptr *bp); // expected-warning{{block pointer is missing a nullability type specifier}}
-// expected-warning@-1{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if the block pointer may be null}}
+// expected-note@-2{{insert '_Nonnull' if the block pointer should never be null}}
+// expected-warning@-3{{pointer is missing a nullability type specifier}}
+// expected-note@-4{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-5{{insert '_Nonnull' if the pointer should never be null}}
 void g4(int (*fp)(int, int));
 void g5(int (**fp)(int, int)); // expected-warning 2{{pointer is missing a nullability type specifier}}
+// expected-note@-1 2 {{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2 2 {{insert '_Nonnull' if the pointer should never be null}}
 
 @interface A(Pragmas1)
 + (instancetype)aWithA:(A *)a;
@@ -57,17 +73,23 @@
 - (void)method4:(NSErrorPtr *)error; // expected-note{{passing argument to parameter 'error' here}}
 - (void)method5:(NSErrorPtrPtr)error;
 // expected-warning@-1{{pointer is missing a nullability type specifier}}
+// expected-note@-2{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-3{{insert '_Nonnull' if the pointer should never be null}}
 
 @property A *aProp;
 @property NSError **anError; // expected-warning 2{{pointer 

[PATCH] D27837: Add fix-it notes to the nullability consistency warning

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8772
+def note_nullability_fix_it : Note<
+  "insert '%select{_Nonnull|_Nullable|_Null_unspecified}0' if the "
+  "%select{pointer|block pointer|member pointer|array parameter}1 "

ahatanak wrote:
> Is the third option (_Null_unspecified) going to be used somewhere? I see the 
> first two options are used in emitNullabilityConsistencyWarning, but not the 
> third one.
I think it's better not to suggest it to people, but it seemed weirder to have 
a reusable diagnostic that's intended to take a NullabilityKind and then have 
it blow up if someone ever decided to use it with NullUnspecified. I can take 
it out if you like.



Comment at: lib/Sema/SemaType.cpp:3491
+/// taking into account whitespace before and after.
+static FixItHint fixItNullability(Sema &S, SourceLocation pointerLoc,
+  NullabilityKind nullability) {

arphaman wrote:
> NIT: I noticed that you don't follow LLVM's naming style for variables here 
> (the parameter and variable names should start with an upper case letter). I 
> realise that there are some functions around the new functions in this file 
> that don't follow this style, so this might be better for local consistency, 
> but it would be nice if the new code follows LLVM's style.
Fair enough, will change. Been working in Swift too much. :-)



Comment at: lib/Sema/SemaType.cpp:3495
+
+  SmallString<32> insertionTextBuf{" "};
+  insertionTextBuf += getNullabilitySpelling(nullability);

arphaman wrote:
> Another NIT: I think it's better to avoid the braced initializer here, and 
> use `SmallString<32> insertionTextBuf = " "` instead
I tried that first, but SmallString doesn't have a valid copy-constructor. If 
it really bugs you I could make it a `+=` line too.


Repository:
  rL LLVM

https://reviews.llvm.org/D27837



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

[PATCH] D27837: Add fix-it notes to the nullability consistency warning

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose updated this revision to Diff 81796.
jordan_rose added a comment.

Updated names to match LLVM style.


Repository:
  rL LLVM

https://reviews.llvm.org/D27837

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaType.cpp
  test/FixIt/Inputs/nullability.h
  test/FixIt/nullability.mm
  test/SemaObjCXX/Inputs/nullability-consistency-1.h
  test/SemaObjCXX/Inputs/nullability-consistency-2.h
  test/SemaObjCXX/Inputs/nullability-consistency-3.h
  test/SemaObjCXX/Inputs/nullability-consistency-4.h
  test/SemaObjCXX/Inputs/nullability-consistency-5.h
  test/SemaObjCXX/Inputs/nullability-consistency-6.h
  test/SemaObjCXX/Inputs/nullability-consistency-7.h
  test/SemaObjCXX/Inputs/nullability-consistency-8.h
  test/SemaObjCXX/Inputs/nullability-consistency-arrays.h
  
test/SemaObjCXX/Inputs/nullability-consistency-system/nullability-consistency-system.h
  test/SemaObjCXX/Inputs/nullability-pragmas-1.h

Index: test/SemaObjCXX/Inputs/nullability-pragmas-1.h
===
--- test/SemaObjCXX/Inputs/nullability-pragmas-1.h
+++ test/SemaObjCXX/Inputs/nullability-pragmas-1.h
@@ -9,6 +9,8 @@
 struct X { };
 
 void f1(int *x); // expected-warning{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2{{insert '_Nonnull' if the pointer should never be null}}
 
 typedef struct __attribute__((objc_bridge(NSError))) __CFError *CFErrorRef;
 typedef NSError *NSErrorPtr;
@@ -39,15 +41,29 @@
 A * _Null_unspecified f16(void);
 void f17(CFErrorRef *error); // expected-note{{no known conversion from 'A * _Nonnull' to 'CFErrorRef  _Nullable * _Nullable' (aka '__CFError **') for 1st argument}}
 void f18(A **); // expected-warning 2{{pointer is missing a nullability type specifier}}
+// expected-note@-1 2 {{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2 2 {{insert '_Nonnull' if the pointer should never be null}}
 void f19(CFErrorRefPtr error); // expected-warning{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2{{insert '_Nonnull' if the pointer should never be null}}
 
 void g1(int (^)(int, int));
 void g2(int (^ *bp)(int, int)); // expected-warning{{block pointer is missing a nullability type specifier}}
-// expected-warning@-1{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if the block pointer may be null}}
+// expected-note@-2{{insert '_Nonnull' if the block pointer should never be null}}
+// expected-warning@-3{{pointer is missing a nullability type specifier}}
+// expected-note@-4{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-5{{insert '_Nonnull' if the pointer should never be null}}
 void g3(block_ptr *bp); // expected-warning{{block pointer is missing a nullability type specifier}}
-// expected-warning@-1{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if the block pointer may be null}}
+// expected-note@-2{{insert '_Nonnull' if the block pointer should never be null}}
+// expected-warning@-3{{pointer is missing a nullability type specifier}}
+// expected-note@-4{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-5{{insert '_Nonnull' if the pointer should never be null}}
 void g4(int (*fp)(int, int));
 void g5(int (**fp)(int, int)); // expected-warning 2{{pointer is missing a nullability type specifier}}
+// expected-note@-1 2 {{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2 2 {{insert '_Nonnull' if the pointer should never be null}}
 
 @interface A(Pragmas1)
 + (instancetype)aWithA:(A *)a;
@@ -57,17 +73,23 @@
 - (void)method4:(NSErrorPtr *)error; // expected-note{{passing argument to parameter 'error' here}}
 - (void)method5:(NSErrorPtrPtr)error;
 // expected-warning@-1{{pointer is missing a nullability type specifier}}
+// expected-note@-2{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-3{{insert '_Nonnull' if the pointer should never be null}}
 
 @property A *aProp;
 @property NSError **anError; // expected-warning 2{{pointer is missing a nullability type specifier}}
+// expected-note@-1 2 {{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2 2 {{insert '_Nonnull' if the pointer should never be null}}
 @end
 
 int *global_int_ptr;
 
 // typedefs not inferred _Nonnull
 typedef int *int_ptr_2;
 
 typedef int * // expected-warning{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if the pointer may be null}}
+// expected-note@-2{{insert '_Nonnull' if the pointer should never be null}}
 *int_ptr_ptr;
 
 static inline void f30(void) {
@@ -90,12 +112,22 @@
 #pragma clang assume_nonnull end
 
 void f20(A *a); // expected-warning{{pointer is missing a nullability type specifier}}
+// expected-note@-1{{insert '_Nullable' if 

[PATCH] D27837: Add fix-it notes to the nullability consistency warning

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose accepted this revision.
jordan_rose added a reviewer: jordan_rose.
jordan_rose added a comment.
This revision is now accepted and ready to land.

Doug was okay with the idea and I trust Alex and Akira would have caught any 
major problems. Committed in r290132.


Repository:
  rL LLVM

https://reviews.llvm.org/D27837



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

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a reviewer: rjmccall.
jordan_rose added a subscriber: rjmccall.
jordan_rose added a comment.

The warning-related parts look reasonable to me.




Comment at: lib/Sema/SemaDecl.cpp:10184
+ (!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
+  VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)) &&
 !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,

This condition's getting complicated, and it shows up in a few places. Would it 
make sense to factor it out?



Comment at: lib/Sema/SemaExpr.cpp:707
   // balance that.
-  if (getLangOpts().ObjCAutoRefCount &&
+  if ((getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) &&
   E->getType().getObjCLifetime() == Qualifiers::OCL_Weak)

This is the one section that //isn't// dealing with the warning. I'd like 
@rjmccall to verify that it's correct.



Comment at: lib/Sema/SemaExpr.cpp:10340
 
-  } else if (getLangOpts().ObjCAutoRefCount) {
+  } else if (getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) {
 checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get());

Does ObjCAutoRefCount imply ObjCWeak? If so, you could just use the latter.


https://reviews.llvm.org/D31005



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

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:10184
+ (!getLangOpts().ObjCAutoRefCount && getLangOpts().ObjCWeak &&
+  VDecl->getType().getObjCLifetime() != Qualifiers::OCL_Weak)) &&
 !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak,

bkelley wrote:
> jordan_rose wrote:
> > This condition's getting complicated, and it shows up in a few places. 
> > Would it make sense to factor it out?
> What do you think about adding a member function like 
> `hasMRCNonTrivialWeakObjCLifetime(const ASTContext &Context)` to QualType to 
> factor out lines 10183-10184? We could use that in D31003, D31004, here, and 
> D31007.
I'm fine with it myself but I don't work on Clang very much anymore. Maybe 
someone else can say whether it's actually a good idea.

(By the way, the conventional abbreviation for this mode is "MRR" for "Manual 
Retain/Release", even though it's "ARC" and "Automated Reference Counting".)



Comment at: lib/Sema/SemaExpr.cpp:10340
 
-  } else if (getLangOpts().ObjCAutoRefCount) {
+  } else if (getLangOpts().ObjCAutoRefCount || getLangOpts().ObjCWeak) {
 checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get());

bkelley wrote:
> jordan_rose wrote:
> > Does ObjCAutoRefCount imply ObjCWeak? If so, you could just use the latter.
> I don't believe so. For Snow Leopard, ARC without weak references was 
> supported so they can be independent. 
Sure, but in that case we don't need the warning, right?


https://reviews.llvm.org/D31005



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

[PATCH] D22391: [Sema] Add warning for implicitly casting a null constant to a non null pointer type

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

This looks like it's only coming up for declarations. What about assignments?

  int x;
  int * _Nonnull p = &x;
  p = NULL; // warn here?




Comment at: lib/Sema/SemaExpr.cpp:7117
+if (E && S.checkNonNullExpr(E))
+  return NullabilityKind::Nullable;
+

This isn't quite correct, unfortunately. `(_Nonnull id)nil` should be 
considered non-nullable, since it's the canonical way to avoid all these 
warnings. It might just be good enough to move this check below the 
`getNullability` one, though.


https://reviews.llvm.org/D22391



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

No, the *arrays themselves* need to be marked as nullable or non-nullable. 
Remember that in C array parameters are passed as pointers.

The compiler should give you a fix-it that shows the correct syntax.


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D26108: Add -Wnullability-completeness-on-arrays.

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

Yep, that's the correct syntax. It's not wonderful, but it's already where the 
C standard lets you put `const`, so we're just following established practice. 
(Learn something new and awful about C every day!)


Repository:
  rL LLVM

https://reviews.llvm.org/D26108



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

[PATCH] D28924: [AST Printer] Print attributes on enum constants

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose created this revision.

The AST printer was dropping attributes on enumerators (enum constants). Now 
it's not.


Repository:
  rL LLVM

https://reviews.llvm.org/D28924

Files:
  lib/AST/DeclPrinter.cpp
  test/Sema/ast-print.c


Index: test/Sema/ast-print.c
===
--- test/Sema/ast-print.c
+++ test/Sema/ast-print.c
@@ -65,3 +65,12 @@
   // CHECK: } z = {(struct Z){}};
   } z = {(struct Z){}};
 }
+
+// CHECK-LABEL: enum EnumWithAttributes {
+enum EnumWithAttributes {
+  // CHECK-NEXT: EnumWithAttributesFoo __attribute__((deprecated(""))),
+  EnumWithAttributesFoo __attribute__((deprecated)),
+  // CHECK-NEXT: EnumWithAttributesBar __attribute__((unavailable(""))) = 50
+  EnumWithAttributesBar __attribute__((unavailable)) = 50
+  // CHECK-NEXT: } __attribute__((deprecated("")))
+} __attribute__((deprecated));
Index: lib/AST/DeclPrinter.cpp
===
--- lib/AST/DeclPrinter.cpp
+++ lib/AST/DeclPrinter.cpp
@@ -438,6 +438,7 @@
 
 void DeclPrinter::VisitEnumConstantDecl(EnumConstantDecl *D) {
   Out << *D;
+  prettyPrintAttributes(D);
   if (Expr *Init = D->getInitExpr()) {
 Out << " = ";
 Init->printPretty(Out, nullptr, Policy, Indentation);


Index: test/Sema/ast-print.c
===
--- test/Sema/ast-print.c
+++ test/Sema/ast-print.c
@@ -65,3 +65,12 @@
   // CHECK: } z = {(struct Z){}};
   } z = {(struct Z){}};
 }
+
+// CHECK-LABEL: enum EnumWithAttributes {
+enum EnumWithAttributes {
+  // CHECK-NEXT: EnumWithAttributesFoo __attribute__((deprecated(""))),
+  EnumWithAttributesFoo __attribute__((deprecated)),
+  // CHECK-NEXT: EnumWithAttributesBar __attribute__((unavailable(""))) = 50
+  EnumWithAttributesBar __attribute__((unavailable)) = 50
+  // CHECK-NEXT: } __attribute__((deprecated("")))
+} __attribute__((deprecated));
Index: lib/AST/DeclPrinter.cpp
===
--- lib/AST/DeclPrinter.cpp
+++ lib/AST/DeclPrinter.cpp
@@ -438,6 +438,7 @@
 
 void DeclPrinter::VisitEnumConstantDecl(EnumConstantDecl *D) {
   Out << *D;
+  prettyPrintAttributes(D);
   if (Expr *Init = D->getInitExpr()) {
 Out << " = ";
 Init->printPretty(Out, nullptr, Policy, Indentation);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28924: [AST Printer] Print attributes on enum constants

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

Good idea, will do.


Repository:
  rL LLVM

https://reviews.llvm.org/D28924



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

[PATCH] D28924: [AST Printer] Print attributes on enum constants

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

Interestingly, this case doesn't actually work yet; we unconditionally print 
enum attributes after the close-brace even though that's not valid for C++ 
attributes. Do you think I should change that as well, or just land this patch 
as is?


Repository:
  rL LLVM

https://reviews.llvm.org/D28924



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

[PATCH] D28924: [AST Printer] Print attributes on enum constants

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose closed this revision.
jordan_rose added a comment.

Landed as-is in https://reviews.llvm.org/rL292571.


Repository:
  rL LLVM

https://reviews.llvm.org/D28924



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

[PATCH] D22391: [Sema] Add warning for implicitly casting a null constant to a non null pointer type

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: include/clang/Basic/DiagnosticGroups.td:290
 def NullableToNonNullConversion : DiagGroup<"nullable-to-nonnull-conversion">;
+def NullConstToNonnull : DiagGroup<"null-const-to-nonnull">;
 def NullabilityCompletenessOnArrays : 
DiagGroup<"nullability-completeness-on-arrays">;

Nitpick: Using "const" here makes me think of the qualifier. Is there a reason 
not to just spell out "constant"?



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8807
+  "implicitly casting a null constant to non-nullable pointer type %0">,
+  InGroup, DefaultIgnore;
+

Why "DefaultIgnore"? This seems like a good warning to be on all the time.


https://reviews.llvm.org/D22391



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

[PATCH] D34955: [Basic] Detect Git submodule version in CMake

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose requested changes to this revision.
jordan_rose added a comment.
This revision now requires changes to proceed.

If I'm remembering correctly from when I set this up, this isn't just about 
detecting which version control system you're using; it's about finding a file 
//that will change on every commit.// Otherwise, the generated Version.cpp 
won't be rebuilt after you update.

If you don't want to go looking for a better choice for this that would handle 
submodules, a stopgap answer would be to add a second entry that just looks for 
`.git` in addition to the one looking for `HEAD`.


https://reviews.llvm.org/D34955



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

[PATCH] D34955: [Basic] Detect Git submodule version in CMake

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

I'm not sure why we would care if a commit is made on a branch. The important 
information here is what commit is checked out, and that's what HEAD refers to.


https://reviews.llvm.org/D34955



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

[PATCH] D34955: [Basic] Detect Git submodule version in CMake

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: lib/Basic/CMakeLists.txt:27
+STRING(REGEX REPLACE "\n" "" file_contents "${file_contents}")
+if ("${file_contents}" MATCHES "^gitdir:")
+  # '.git' is indeed a link to the submodule's Git directory.

How about

```
if("${file_contents}" MATCHES "^gitdir: ([^\n]+)")
  set(git_path "${git_path}${CMAKE_MATCH_0}")
endif()
```

and then no stripping or replacing later at all? This admittedly doesn't handle 
some future submodule format that doesn't put `gitdir` on the first line, but 
neither does the code that's there.

(This also doesn't handle absolute path submodule references, but looking 
around a bit makes it sound like those aren't supposed to occur in practice and 
are considered bugs when git does generate them. I could be wrong about that 
though.)


https://reviews.llvm.org/D34955



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

[PATCH] D34955: [Basic] Detect Git submodule version in CMake

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose accepted this revision.
jordan_rose added a comment.
This revision is now accepted and ready to land.

Whoops, yes, this new version looks good!


https://reviews.llvm.org/D34955



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

[PATCH] D35533: [Basic] Update CMakeLists.txt to handle Repo

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

As commented at https://reviews.llvm.org/D34955#798369, this isn't sufficient 
because it doesn't force a rebuild when new changes are merged in from 
upstream. It's not //harmful// to stick with the old version information, but 
if you can find something better that would be, um, better.


https://reviews.llvm.org/D35533



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

[PATCH] D35533: [Basic] Update CMakeLists.txt to handle Repo

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a subscriber: modocache.
jordan_rose added a comment.

> The addition of "${path}/.git/HEAD" at line 37 is used only once when repo is 
> initially synced "for the first time". This is because ${git_path}/logs/HEAD 
> file at line 34 for Git or Git submodule does not exist with initial repo 
> sync[.]

Hm, interesting. Unfortunately, adding more commits won't automatically force a 
reconfigure. Maybe we should consider adding //all// of these as dependencies? 
cc @modocache, who just changed some of this.


https://reviews.llvm.org/D35533



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