[PATCH] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added a comment.

Yes, I think it's reasonable to treat instancetype as an inherited requirement. 
I guess the only exception would be if we had some notion of final classes or 
methods in Objective-C, in which case they'd be able to return a concrete type.


Repository:
  rL LLVM

https://reviews.llvm.org/D36790



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

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

[Doug Gregor via Phabricator via cfe-commits]

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

One minor request about regarding a dyn_cast, but this looks like the right 
approach. Thank you!




Comment at: clang/lib/AST/Type.cpp:1293
+
+const auto *newAttrType = dyn_cast(newType.getTypePtr());
+if (newAttrType->getAttrKind() != attr::ObjCKindOf)

Either this should be a `cast` or the next if should check for 
!newAttrType. I suspect the latter is safer.


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] D37089: [Sema] Error out early for tags defined inside an enumeration.

[Doug Gregor via Phabricator via cfe-commits]

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

Looks good to me; sorry for the delay.


https://reviews.llvm.org/D37089



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

[PATCH] D44670: [CXX] Templates specialization visibility can be wrong

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added inline comments.



Comment at: lib/AST/Decl.cpp:1078
+for (const auto *RD :
+ spec->getSpecializedTemplate()->getTemplatedDecl()->redecls()) {
+  auto Vis = getVisibilityOf(RD, kind);

Do we want to look at *all* redeclarations, or only those declarations that 
precede the declaration that we found? The latter seems more correct, but IIRC 
visibility has often been able to "look forward" to declarations that come 
later in the translation unit.


Repository:
  rC Clang

https://reviews.llvm.org/D44670



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

[PATCH] D44670: [CXX] Templates specialization visibility can be wrong

[Doug Gregor via Phabricator via cfe-commits]

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

LGTM, thanks


Repository:
  rC Clang

https://reviews.llvm.org/D44670



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added a comment.

In D84005#2171982 , @MForster wrote:

> @milseman, @doug.gregor, could you please help with the open questions on 
> this review?
>
> Specifically:
>
> - Is `ns_error_domain` ever needed for something other than an enum?


No, it only makes sense on enums.

> - Why is this designed in the way it is (requiring an identifier for the 
> domain, not allowing literals and then only using the name of the identifier 
> from Swift)?

It's codifying the design of NSError, which has been this way since... longer 
than Clang has existed, and is independent of Swift. NSErrors have a domain 
(identified by an NSString constant symbol, not a literal, for pointer 
uniqueness and code size)  and a code (an integer, conventionally defined by an 
enum). The two need to be used together, e.g., you create an NSError with a 
domain and a code from that domain. This attribute finally ties those things 
together at the source level.

This is leads to the answer to Aaron's question:

> Are there plans to use this attribute in Clang itself?

It would absolutely make sense to add some warnings if you've mismatched your 
domain and code when constructing an NSError (e.g., uses of 
https://developer.apple.com/documentation/foundation/nserror/1522782-errorwithdomain?language=objc)
 or even if when testing an error in an "if" statement (if checking both domain 
and code, make sure the code enumerator is from the same domain). I bet you'd 
catch some fiddly error-handling bugs this way.

> - Is it ok to make this attribute inheritable?

Sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D130205: [Darwin toolchain] Tune the logic for finding arclite.

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor created this revision.
doug.gregor added a reviewer: arphaman.
doug.gregor added a project: clang.
Herald added a project: All.
doug.gregor requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.

The heuristic used to determine where the arclite libraries are to be
found was based on the path of the `clang` executable. However, in some
scenarios the `clang` executable is within a toolchain that does not
have arclite. When this happens, derive the arclite paths from the
sysroot option.

This allows Clang to correctly derive the arclite directory in, e.g.,
Swift CI, using similar logic to what the Swift driver has been doing
for several years.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130205

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp


Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -1141,25 +1141,38 @@
   SmallString<128> P(getDriver().ClangExecutable);
   llvm::sys::path::remove_filename(P); // 'clang'
   llvm::sys::path::remove_filename(P); // 'bin'
+  llvm::sys::path::append(P, "lib", "arc");
 
   // 'libarclite' usually lives in the same toolchain as 'clang'. However, the
   // Swift open source toolchains for macOS distribute Clang without 
libarclite.
   // In that case, to allow the linker to find 'libarclite', we point to the
   // 'libarclite' in the XcodeDefault toolchain instead.
-  if (getXcodeDeveloperPath(P).empty()) {
-if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
+  if (!getVFS().exists(P)) {
+auto updatePath = [&](const Arg *A) {
   // Try to infer the path to 'libarclite' in the toolchain from the
   // specified SDK path.
   StringRef XcodePathForSDK = getXcodeDeveloperPath(A->getValue());
-  if (!XcodePathForSDK.empty()) {
-P = XcodePathForSDK;
-llvm::sys::path::append(P, "Toolchains/XcodeDefault.xctoolchain/usr");
-  }
+  if (XcodePathForSDK.empty())
+return false;
+
+  P = XcodePathForSDK;
+  llvm::sys::path::append(P, "Toolchains/XcodeDefault.xctoolchain/usr",
+  "lib", "arc");
+  return getVFS().exists(P);
+};
+
+bool updated = false;
+if (const Arg *A = Args.getLastArg(options::OPT_isysroot))
+  updated = updatePath(A);
+
+if (!updated) {
+  if (const Arg *A = Args.getLastArg(options::OPT__sysroot_EQ))
+updatePath(A);
 }
   }
 
   CmdArgs.push_back("-force_load");
-  llvm::sys::path::append(P, "lib", "arc", "libarclite_");
+  llvm::sys::path::append(P, "libarclite_");
   // Mash in the platform.
   if (isTargetWatchOSSimulator())
 P += "watchsimulator";


Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -1141,25 +1141,38 @@
   SmallString<128> P(getDriver().ClangExecutable);
   llvm::sys::path::remove_filename(P); // 'clang'
   llvm::sys::path::remove_filename(P); // 'bin'
+  llvm::sys::path::append(P, "lib", "arc");
 
   // 'libarclite' usually lives in the same toolchain as 'clang'. However, the
   // Swift open source toolchains for macOS distribute Clang without libarclite.
   // In that case, to allow the linker to find 'libarclite', we point to the
   // 'libarclite' in the XcodeDefault toolchain instead.
-  if (getXcodeDeveloperPath(P).empty()) {
-if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
+  if (!getVFS().exists(P)) {
+auto updatePath = [&](const Arg *A) {
   // Try to infer the path to 'libarclite' in the toolchain from the
   // specified SDK path.
   StringRef XcodePathForSDK = getXcodeDeveloperPath(A->getValue());
-  if (!XcodePathForSDK.empty()) {
-P = XcodePathForSDK;
-llvm::sys::path::append(P, "Toolchains/XcodeDefault.xctoolchain/usr");
-  }
+  if (XcodePathForSDK.empty())
+return false;
+
+  P = XcodePathForSDK;
+  llvm::sys::path::append(P, "Toolchains/XcodeDefault.xctoolchain/usr",
+  "lib", "arc");
+  return getVFS().exists(P);
+};
+
+bool updated = false;
+if (const Arg *A = Args.getLastArg(options::OPT_isysroot))
+  updated = updatePath(A);
+
+if (!updated) {
+  if (const Arg *A = Args.getLastArg(options::OPT__sysroot_EQ))
+updatePath(A);
 }
   }
 
   CmdArgs.push_back("-force_load");
-  llvm::sys::path::append(P, "lib", "arc", "libarclite_");
+  llvm::sys::path::append(P, "libarclite_");
   // Mash in the platform.
   if (isTargetWatchOSSimulator())
 P += "watchsimulator";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36530: [Parse] Document PrintStats, SkipFunctionBodies

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added a comment.

LGTM, thank you!


Repository:
  rL LLVM

https://reviews.llvm.org/D36530



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

[PATCH] D87534: Sema: introduce `__attribute__((__swift_name__))`

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added a comment.

Thank you for doing this!




Comment at: clang/include/clang/Basic/Attr.td:2173
+  SubjectList<[Enum, EnumConstant, Field, Function, GlobalVar, Struct, 
TypedefName,
+   ObjCInterface, ObjCClassMethod, ObjCInstanceMethod, 
ObjCProperty, ObjCProtocol],
+  ErrorDiag, "ExpectedSwiftNameSubjects">;

compnerd wrote:
> Note for @rjmccall and @doug.gregor - this version actually enumerates the 
> subjects to which this attribute appertains unlike what was in the original 
> swift version.  Are there other subjects which this should list?
Hmm. If we enumerate the subjects, we're going to have to update Clang whenever 
Swift's Clang importer learns a new trick. For example, this is probably 
missing CXXMethod and FunctionTmpl based on work that's going on in Swift. I 
suspect we're also missing ObjCCompatibilityAlias. I'm inclined to treat this 
more like AsmLabelAttr and not try to enumerate subjects at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87534

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

[PATCH] D92495: [clang] Add a new nullability annotation for swift async: _Nullable_result

[Doug Gregor via Phabricator via cfe-commits]

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

This looks great to me, thank you!


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

https://reviews.llvm.org/D92495

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

[PATCH] D92495: [clang] Add a new nullability annotation for swift async: _Nullable_result

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added inline comments.



Comment at: clang/test/SemaObjC/nullable-result.m:6
+@class NSError;
+
+@interface SomeClass

Can you add a `__has_attribute` check for `_Nullable_result`?


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

https://reviews.llvm.org/D92495

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

[PATCH] D92355: [clang] add a `swift_async_name` attribute

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor accepted this revision.
doug.gregor added a comment.

Thank you!


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

https://reviews.llvm.org/D92355

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

[PATCH] D92742: [clang] Add support for attribute 'swift_async'

[Doug Gregor via Phabricator via cfe-commits]

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

This looks good. I went ahead and tested it as part of adoption in Swift's 
Clang importer, over at https://github.com/apple/swift/pull/34985


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

https://reviews.llvm.org/D92742

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

[PATCH] D69420: [clang][clang-scan-deps] Add support for extracting full module dependencies.

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added a comment.

It looks like Inputs/modules_cdb.json missing from this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69420



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

[PATCH] D69420: [clang][clang-scan-deps] Add support for extracting full module dependencies.

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added a comment.

In D69420#1757664 , @Bigcheese wrote:

> It was already there from a previous commit. I see it in tree currently.


Oh, sorry. I missed it in my cherry-picking and GitHub search failed to find it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69420



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

[PATCH] D28349: [Frontend] The macro that describes the Objective-C bool type should be defined in C as well

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added a comment.

The "!LangOpts.CPlusPlus" doesn't make sense to me. You're presumably defining 
this macro in C because the Objective-C runtime is usable from C, but that same 
logic applies to C++ code. It seems like we should be defining this macro 
unconditionally.


Repository:
  rL LLVM

https://reviews.llvm.org/D28349



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

[PATCH] D31378: [PCH] Attach instance's dependency collectors to PCH external AST sources.

[Doug Gregor via Phabricator via cfe-commits]

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

LGTM!


https://reviews.llvm.org/D31378



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

[PATCH] D34741: [ASTReader] Treat multiple defns of ObjC protocols the same as interfaces.

[Doug Gregor via Phabricator via cfe-commits]

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

Looks great!


https://reviews.llvm.org/D34741



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

[PATCH] D35923: Fix PR32332 - PCM nondeterminism with builtins caused by global module index

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added a comment.

I am *very* concerned about this. The optimization that avoids doing lookups 
within module files where we "know" we won't find anything was very important 
at the time it was introduced, so I'd like to understand better why it didn't 
make a significant difference in your benchmarks before we disable it. 
(Especially because of built-ins; those are Very Weird and probably modeled 
wrong).


https://reviews.llvm.org/D35923



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

[PATCH] D27580: [modules] Add optional out-param to ASTReader::ReadAST for imported submodules.

[Doug Gregor via Phabricator via cfe-commits]

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

LGTM!


https://reviews.llvm.org/D27580



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

[PATCH] D109451: Summary: Introduce a new frontend flag `-fswift-async-fp={auto|always|never}` that controls how code generation sets the Swift extended async frame info bit. There are three possibili

[Doug Gregor via Phabricator via cfe-commits]

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

Looks great, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109451

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