[PATCH] D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.

In https://reviews.llvm.org/D50719#1199427, @mclow.lists wrote:

> I have pissed in this area, too - See 
> https://bugs.llvm.org/show_bug.cgi?id=38495 and the proposed resolution here: 
> https://bugs.llvm.org/attachment.cgi?id=20692
>  How about I just make this change as part of that fix?


If the bots are red from a previous commit I think it would be better for Louis 
to commit this separately.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50719



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: libcxx/include/__config:798-804
 #ifndef _LIBCPP_HIDE_FROM_ABI
-#  define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  ifdef _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
+#  else
+#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE
+#  endif
 #endif

This doesn't leave a user-friendly way of opting out of internal linkage if a 
vendor decides to turn it on by default.  I suggest adding a layer of 
indirection:
```
// Elsewhere.
#cmakedefine _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE_BY_DEFAULT

// Here.
#ifndef _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE
#  ifdef _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE_BY_DEFAULT
#define _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE 1
#  else
#define _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE 0
#  endif
#endif

#ifndef _LIBCPP_HIDE_FROM_ABI
#  if _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE
#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE
#  else
#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_ALWAYS_INLINE
#  endif
#endif
```
Then users can set the behaviour with 
`-D_LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE=1` or 
`-D_LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE=0`, overriding the vendor default.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

LGTM if thakis is happy.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50652



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

[PATCH] D46694: [diagtool] Install diagtool

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This SGTM, but I wouldn't mind hearing from others.  I wonder if this is worth 
a quick RFC on cfe-dev?


Repository:
  rC Clang

https://reviews.llvm.org/D46694



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

[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

The code looks correct to me, although I have a few suggestions inline.  When 
you resubmit, please include full context (e.g., `git diff -U99`).

Jan and I discussed this ahead of time and I agree with adding the assertion as 
the message.  However, I'd rather hear from others before signing off.




Comment at: lib/Sema/SemaDeclCXX.cpp:13739-13740
   SmallString<256> MsgBuffer;
-  llvm::raw_svector_ostream Msg(MsgBuffer);
-  if (AssertMessage)
-AssertMessage->printPretty(Msg, nullptr, getPrintingPolicy());
+  {
+llvm::raw_svector_ostream Msg(MsgBuffer);
+if (AssertMessage)

Is this extra nesting necessary?



Comment at: lib/Sema/SemaDeclCXX.cpp:13744-13746
+  AssertExpr->printPretty(Msg, nullptr, getPrintingPolicy());
+  MsgBuffer.insert(MsgBuffer.begin(), '\"');
+  MsgBuffer.append(1, '\"');

Inserting to the beginning of the buffer seems a bit weird, especially since 
you're jumping between `Msg` and `MsgBuffer`.

This feels more natural to me:

```
Msg << '"';
AssertExpr->printPretty(Msg, nullptr, getPrintingPolicy());
Msg << '"';
```



Comment at: lib/Sema/SemaDeclCXX.cpp:13761
 Diag(StaticAssertLoc, diag::err_static_assert_failed)
-  << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
+  << MsgBuffer.empty() << MsgBuffer.str()
+  << AssertExpr->getSourceRange();

Is there always a message now?  If so, can you just assert that before the `if` 
statement and simplify this code?


Repository:
  rC Clang

https://reviews.llvm.org/D46834



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

[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

(Somehow I missed Richard's comment when I replied last night, even though it 
preceded mine by 5 hours...)

In https://reviews.llvm.org/D46834#1098727, @rsmith wrote:

> This would violate our guidelines for diagnostic messages; see 
> https://clang.llvm.org/docs/InternalsManual.html#the-format-string
>
> In particular: "Take advantage of location information. The user will be able 
> to see the line and location of the caret, so you don’t need to tell them 
> that the problem is with the 4th argument to the function: just point to it."
>
> > Reasons why printing assert expression in absence of string literal is 
> > useful are:
> > 
> > 1. Serialized diagnostics (--serialize-diagnostics) don't contain code 
> > snippet and thus error message lack context.
> > 2. Not all tools scraping clang diagnostics use caret snippet provided by 
> > -fcaret-diagnostics.
>
> This seems like an excellent reason to fix those tools -- per our 
> documentation, Clang's diagnostics are not designed to be used without the 
> caret and snippet.
>
> If you want to change our diagnostic policy from "the diagnostic text plus 
> the line and location of the caret should be enough to identify the problem; 
> do not repeat information in the diagnostic that the caret portion shows" to 
> "the diagnostic text alone should be sufficient", that's certainly something 
> we can discuss, but you'll need to make the case for why that's a good change 
> of policy.


I wasn't aware of the policy.  I can't convince myself that `static_assert` 
should be an exception to it.

It's ironic, though.  What I'm concerned about are interfaces like a Vim 
location list, a condensed view of diagnostics that won't show the source until 
you select a particular diagnostic.  The status quo may lead some users to 
leverage a macro like this to hack the location list view:

  #define MY_STATIC_ASSERT(X) static_assert(X, #X)

getting rid of which was the motivation for the single-argument `static_assert` 
in the first place.


Repository:
  rC Clang

https://reviews.llvm.org/D46834



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

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a reviewer: vsapsai.
dexonsmith added a comment.

In https://reviews.llvm.org/D17741#1067816, @weimingz wrote:

> split the original into two parts. This one supports 
> -ffile-macro-prefix-to-remove function.
>
> I locally verified it.  To add a test case, do we need to use VFS?


VFS might work.


https://reviews.llvm.org/D17741



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

[PATCH] D40864: [Darwin] Add a new -mstack-probe option and enable by default

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Did this eventually go in?


Repository:
  rC Clang

https://reviews.llvm.org/D40864



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

[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D46834#1102391, @rsmith wrote:

> In https://reviews.llvm.org/D46834#1101586, @jkorous wrote:
>
> > We reconsidered this in light of the policy - thanks for pointing that out 
> > Richard! 
> >  Just to be sure that I understand it - the policy is meant for CLI and not 
> > serialized diagnostics, right?
>
>
> The policy certainly seems designed around the CLI use case. For serialized 
> diagnostics, it would make sense to either serialize the snippet or enough 
> information that the snippet can be reconstructed. And if that can't be done, 
> or fails to satisfy some other use case, then we should discuss how we 
> proceed -- for instance, we could consider having different diagnostic 
> messages for the case where we have a snippet and for the case where we do 
> not.


Right.  There are places in the IDE where there is a condensed view of all 
diagnostics (like a Vim location list), and others where the diagnostics are 
shown inline with the sources.  I think what we want is an optional auxiliary 
record/field in a diagnostic with that contains context for when the source 
context is missing, and then the IDE can choose which to display.  It's 
optional because most diagnostics are good enough as is for location lists.


Repository:
  rC Clang

https://reviews.llvm.org/D46834



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

[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D46834#1102407, @rsmith wrote:

> In https://reviews.llvm.org/D46834#1102395, @dexonsmith wrote:
>
> > In https://reviews.llvm.org/D46834#1102391, @rsmith wrote:
> >
> > > The policy certainly seems designed around the CLI use case. For 
> > > serialized diagnostics, it would make sense to either serialize the 
> > > snippet or enough information that the snippet can be reconstructed. And 
> > > if that can't be done, or fails to satisfy some other use case, then we 
> > > should discuss how we proceed -- for instance, we could consider having 
> > > different diagnostic messages for the case where we have a snippet and 
> > > for the case where we do not.
> >
> >
> > Right.  There are places in the IDE where there is a condensed view of all 
> > diagnostics (like a Vim location list), and others where the diagnostics 
> > are shown inline with the sources.  I think what we want is an optional 
> > auxiliary record/field in a diagnostic with that contains context for when 
> > the source context is missing, and then the IDE can choose which to 
> > display.  It's optional because most diagnostics are good enough as is for 
> > location lists.
>
>
> That sounds good to me. I think it would also make sense to use the alternate 
> form for the CLI case if the user is using `-fno-caret-diagnostics` for some 
> reason.


Yes that sounds right.


Repository:
  rC Clang

https://reviews.llvm.org/D46834



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

[PATCH] D46485: Add python tool to dump and construct header maps

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: test/Preprocessor/headermap-rel.c:2-3
 
 // This uses a headermap with this entry:
 //   Foo.h -> Foo/Foo.h
 

Should we delete this comment now that the header map is human-readable?



Comment at: test/Preprocessor/headermap-rel.c:6
+// RUN: rm -f %t.hmap
+// RUN: hmaptool write %S/Inputs/headermap-rel/foo.hmap.json %t.hmap
+// RUN: %clang_cc1 -E %s -o %t.i -I %t.hmap -F %S/Inputs/headermap-rel

Should there be some sort of `REQUIRES:` clause for using `hmaptool`?


https://reviews.llvm.org/D46485



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

[PATCH] D46485: Add python tool to dump and construct header maps

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Removing the binary header map files is a clear win, but I'd like someone else 
to approve this.




Comment at: test/Preprocessor/headermap-rel2.c:1-2
 // This uses a headermap with this entry:
 //   someheader.h -> Product/someheader.h
 

This comment also seems unnecessary.


https://reviews.llvm.org/D46485



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: lib/Lex/HeaderSearch.cpp:753-754
+  IncluderAndDir.second->getName()))
+Diags.Report(IncludeLoc,
+ diag::warn_quoted_include_in_framework_header)
+<< Filename;

bruno wrote:
> aaron.ballman wrote:
> > This seems like a good place for a fix-it to switch the include style. Is 
> > there a reason to not do that work for the user?
> Like I explained above, we don't know which framework the header could be 
> part of, so a fix-it could be misleading.
Clang supports editor placeholders, which we use in some refactoring-style 
fix-its.  I think this would be spelled `<#framework-name#>`, or `#include 
<<#framework-name#>/Foo.h>`


Repository:
  rC Clang

https://reviews.llvm.org/D47157



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

[PATCH] D51240: Add a flag to remap manglings when reading profile data information.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a reviewer: vsk.
dexonsmith added a comment.

In https://reviews.llvm.org/D51240#1213179, @davidxl wrote:

> In particular, I don't see much need to do this for instrumentation based PGO 
> [...]


Why not?


https://reviews.llvm.org/D51240



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

[PATCH] D51240: Add a flag to remap manglings when reading profile data information.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D51240#1213358, @davidxl wrote:

> Re  "Why not":
>
> The common use model of instrumentation based PGO and SamplePGO are quite 
> different. The former usually uses 'fresh' profile that matches the source. 
> If there are massive code refactoring or ABI changes, the user can simply 
> regenerate the profile.  For the latter, the profile data is usually 
> collected from production binaries, so the source is almost always newer than 
> the profile.


Frontend-based instrumentation is designed to degrade gracefully as the source 
changes.  I don't follow your conclusion that the profile is necessarily fresh. 
 (I'm fine with the patch being split though.)


https://reviews.llvm.org/D51240



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

[PATCH] D30009: Add support for '#pragma clang attribute'

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a subscriber: erik.pilkington.
dexonsmith added a comment.

In https://reviews.llvm.org/D30009#1218647, @rsmith wrote:

> One consequence of this patch (see https://reviews.llvm.org/rL341002) is that 
> adding documentation to an existing attribute //changes the behavior of 
> Clang//. That does not seem acceptable to me: documentation improvements 
> should not change which attributes we allow this pragma to appertain to.
>
> If we really want an "only documented attribtues can use this feature" rule 
> (and I'm really not convinced that is a useful rule to enforce


I also don't quite see why this is useful.

> ), I think the best way to do that is to add another flag on the Attr 
> instance in the .td file to opt the attribute out of `#pragma clang` support, 
> opt out all the undocumented attributes, and add an error to TableGen if we 
> find that an undocumented attribute has `#pragma clang` support enabled.

It seems arbitrary not to support an attribute from `#pragma clang attribute` 
just because it was once undocumented.  It does seem incrementally better, but 
still not great.

(FYI, @arphaman is on vacation until next week so he's unlikely to respond 
promptly.)


Repository:
  rL LLVM

https://reviews.llvm.org/D30009



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

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:50
 // CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
+// CHECK-NEXT: ExtVectorType (SubjectMatchRule_type_alias)
 // CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule_record, 
SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, 
SubjectMatchRule_function, SubjectMatchRule_namespace, 
SubjectMatchRule_objc_category, SubjectMatchRule_objc_interface, 
SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, 
SubjectMatchRule_objc_protocol, SubjectMatchRule_record, 
SubjectMatchRule_type_alias, SubjectMatchRule_variable))

rsmith wrote:
> I think `__attribute__((ext_vector_type))` is a good candidate to *not* have 
> `#pragma clang attribute` support. A type modifier like this really should 
> only be declared as part of declaring the type. Opinions?
The same argument might hold for most type attributes.  I have trouble 
imagining someone using this, but it's also hard to see how supporting it would 
lead to bugs in user code.  It seems a bit simpler to support everything.

I don't have a strong opinion though.


Repository:
  rC Clang

https://reviews.llvm.org/D51507



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

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:50
 // CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
+// CHECK-NEXT: ExtVectorType (SubjectMatchRule_type_alias)
 // CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule_record, 
SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, 
SubjectMatchRule_function, SubjectMatchRule_namespace, 
SubjectMatchRule_objc_category, SubjectMatchRule_objc_interface, 
SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, 
SubjectMatchRule_objc_protocol, SubjectMatchRule_record, 
SubjectMatchRule_type_alias, SubjectMatchRule_variable))

aaron.ballman wrote:
> dexonsmith wrote:
> > rsmith wrote:
> > > I think `__attribute__((ext_vector_type))` is a good candidate to *not* 
> > > have `#pragma clang attribute` support. A type modifier like this really 
> > > should only be declared as part of declaring the type. Opinions?
> > The same argument might hold for most type attributes.  I have trouble 
> > imagining someone using this, but it's also hard to see how supporting it 
> > would lead to bugs in user code.  It seems a bit simpler to support 
> > everything.
> > 
> > I don't have a strong opinion though.
> I don't think `#pragma clang attribute` should apply to types -- types show 
> up in far too many places and attributes on types changes the fundamental 
> meaning of types a bit too much for my tastes. I'd prefer users annotate the 
> type directly.
> types show up in far too many places and attributes on types changes the 
> fundamental meaning of types a bit too much for my tastes

I don't see how region-based attributes on type aliases is really any different 
from region-based annotations on variables.

Moreover, I'm strongly against disallowing use of this pragma on type aliases 
in general: as a vendor, we've already shipped that support for a number of 
attributes.  Most critically, ExternalSourceSymbol applies to type aliases, and 
handling ExternalSourceSymbol was our primary motivation for adding this 
feature (the alternative was to add yet-another-attribute-specific-`#pragma`).



Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:100
+// CHECK-NEXT: ObjCReturnsInnerPointer (SubjectMatchRule_objc_method, 
SubjectMatchRule_objc_property)
+// CHECK-NEXT: ObjCRootClass (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ObjCRuntimeName (SubjectMatchRule_objc_interface, 
SubjectMatchRule_objc_protocol)

kristina wrote:
> rsmith wrote:
> > kristina wrote:
> > > There is only one root class in a given runtime (ie. NSObject for objc4, 
> > > or Object for older Apple runtimes, ObjFW also has a different root 
> > > class). Having more than one makes no sense especially in the same 
> > > lexical context as there are no use cases I can think of where you would 
> > > have more than one active ObjC runtime within a process.
> > Thanks. Yes, this attribute probably doesn't make very much sense to use in 
> > conjunction with the pragma.
> > 
> > So, do we explicitly disallow it, or do we allow it with the expectation 
> > that it's likely that no-one will ever want to use it? (That is, do we want 
> > to disallow cases that are not useful, or merely cases that are not 
> > meaningful?) I don't have a strong opinion here, but I'm mildly inclined 
> > towards allowing the pragma on any attribute where it's meaningful, as 
> > there may be useful uses that we just didn't think of, and it costs nothing 
> > to permit.
> Yes in theory it could only be used on a single interface in which case it 
> would be perfectly valid. Otherwise when used incorrectly it would issue a 
> diagnostic. As per your inclination of allowing it for all attributes I would 
> say leave it allowed, as it **can** be used in a correct way. Diagnostics are 
> sufficient enough to point out when it happens to apply the attribute to more 
> than one interface.
> 
> So given your comment, I would say leave it as allowed.
> So, do we explicitly disallow it, or do we allow it with the expectation that 
> it's likely that no-one will ever want to use it? (That is, do we want to 
> disallow cases that are not useful, or merely cases that are not meaningful?) 

I'd prefer to only disallow cases that are not meaningful.


Repository:
  rC Clang

https://reviews.llvm.org/D51507



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

[PATCH] D51440: [ToolChains] Link to compiler-rt with -L + -l when possible

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D51440#1225318, @steven_wu wrote:

> I do prefer the current approach especially on Darwin. Some concerns of 
> switching to use "-L + -l" are:
>
> 1. clang and compiler-rt are rev-locked. Inferring the compiler-rt from 
> resource-dir and passing to linker with the full path can prevent mistakes of 
> mixing them up.
> 2. This change does change linker semantics on Darwin. ld64 treats the inputs 
> from command line with highest priority. Currently ld64 will use compiler-rt 
> symbols before any other libraries passed in with -l flag. If clang decide to 
> pass compiler-rt with -l flag, any other libraries (static or dynamic) that 
> passed with -l flag will takes the priority over compiler-rt. This can lead 
> to unexpected behavior.


I tend to agree with Steven.  I'd rather avoid a semantic change here.


Repository:
  rC Clang

https://reviews.llvm.org/D51440



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

[PATCH] D51789: [WIP][clang] Add the no_extern_template attribute

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith edited subscribers, added: cfe-commits; removed: llvm-commits.
dexonsmith added a comment.

+cfe-commits
-llvm-commits


Repository:
  rL LLVM

https://reviews.llvm.org/D51789



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

[PATCH] D49808: [libc++] Factor duplicate code into function templates

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

Nice cleanup!  LGTM.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49808



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

[PATCH] D49914: [libc++] Add the _LIBCPP_HIDE_FROM_ABI_AFTER_V1 macro

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This looks correct to me, but I wouldn't mind having someone else take a look 
too.




Comment at: libcxx/include/__config:798
 
-// Just so we can migrate to _LIBCPP_HIDE_FROM_ABI gradually.
-#define _LIBCPP_INLINE_VISIBILITY _LIBCPP_HIDE_FROM_ABI
-
-#ifndef _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
-#  if !defined(_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS)
-#define _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY 
__attribute__((__visibility__("default"), __always_inline__))
+#ifdef _LIBCPP_BUILDING_LIBRARY
+#  if _LIBCPP_ABI_VERSION > 1

It looks like if you switch this to `#if !defined(...)` you can use `#elif` 
instead of a nested `#if`.  I think that would make the logic a bit more clear 
for me, but if you disagree feel free to leave it as is.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49914



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

[PATCH] D33368: [libcxxabi][demangler] Fix a crash in the demangler

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: src/cxa_demangle.cpp:3036
 break;
-if (db.names.size() < 2)
+assert(k0 <= k1 && "parse_type() mutated the name stack");
+if (k1 == k0)

There's no `#include ` in this file.  Depending on the standard 
library you're building against, you made need that here.


https://reviews.llvm.org/D33368



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

[PATCH] D28404: IRGen: Add optnone attribute on function during O0

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

Actually, looking through the comments, it appears that everyone (eventually) 
agreed with the approach in the patch.  I agree too.  LGTM.

Mehdi, are you able to rebase and commit, or should someone take over?


https://reviews.llvm.org/D28404



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

[PATCH] D33739: [Sema] Improve -Wstrict-prototypes diagnostic message for blocks

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

LGTM.


https://reviews.llvm.org/D33739



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

[PATCH] D33977: [libcxx][WIP] Experimental support for a scheduler for use in the parallel stl algorithms

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

You mention that the lock-free `deque` gives a speedup.  I agree that should be 
left for a follow-up, but what kind of speedup did it give?

Have you measured whether the parallel algorithm gives a speedup, compared to a 
non-parallel algorithm?


https://reviews.llvm.org/D33977



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

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This is the right idea, although it only covers macOS.

Any ideas for how to test this?




Comment at: src/experimental/filesystem/operations.cpp:23-28
+// We can use the presence of UTIME_OMIT to detect platforms that do not
+// provide utimensat, with some exceptions on OS X.
+#if !defined(UTIME_OMIT) || \
+   (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && 
__MAC_OS_X_VERSION_MIN_REQUIRED < 1030)
+#define _LIBCPP_HAS_NO_UTIMENSAT
+#endif

Sadly this isn't quite sufficient.  As per Jack's suggested SDK patch in the 
PR, we need to enumerate the platforms :/.  I think this should be the right 
logic for the four Darwin platforms:

(defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && 
__IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_11_0) || \
(defined(__WATCH_OS_VERSION_MIN_REQUIRED) &&  
__WATCH_OS_VERSION_MIN_REQUIRED < __WATCHOS_4_0)  || \
(defined(__TV_OS_VERSION_MIN_REQUIRED) && __TV_OS_VERSION_MIN_REQUIRED < 
__TVOS_11_0)   || \
(defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && 
__MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_13)



https://reviews.llvm.org/D34249



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

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: src/experimental/filesystem/operations.cpp:23-28
+// We can use the presence of UTIME_OMIT to detect platforms that do not
+// provide utimensat, with some exceptions on OS X.
+#if !defined(UTIME_OMIT) || \
+   (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && 
__MAC_OS_X_VERSION_MIN_REQUIRED < 1030)
+#define _LIBCPP_HAS_NO_UTIMENSAT
+#endif

EricWF wrote:
> dexonsmith wrote:
> > Sadly this isn't quite sufficient.  As per Jack's suggested SDK patch in 
> > the PR, we need to enumerate the platforms :/.  I think this should be the 
> > right logic for the four Darwin platforms:
> > 
> > (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && 
> > __IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_11_0) || \
> > (defined(__WATCH_OS_VERSION_MIN_REQUIRED) &&  
> > __WATCH_OS_VERSION_MIN_REQUIRED < __WATCHOS_4_0)  || \
> > (defined(__TV_OS_VERSION_MIN_REQUIRED) && __TV_OS_VERSION_MIN_REQUIRED 
> > < __TVOS_11_0)   || \
> > (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && 
> > __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_13)
> > 
> Do we have to do the below dance for all of those macros?
> 
> ```
> #if !defined(__FOO_VERSION_MIN_REQUIRED) && 
> defined(__ENVIROMENT_FOO_VERSION_MIN_REQUIRED)
> #define __FOO_VERSION_MIN_REQUIRED __ENVIROMENT_FOO_VERSION_REQUIRED
> #endif
> ```
Nope.  I just advised you to use the wrong ones.  Use the `__ENVIRONMENT` 
versions pre-defined by Clang.



Comment at: src/experimental/filesystem/operations.cpp:22-24
+#if defined(__APPLE__)
+#include 
+#endif

I only just noticed you were including Availability.h.  That shouldn't be 
necessary, since the macros should be defined by the compiler.


https://reviews.llvm.org/D34249



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

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: src/experimental/filesystem/operations.cpp:23-28
+// We can use the presence of UTIME_OMIT to detect platforms that do not
+// provide utimensat, with some exceptions on OS X.
+#if !defined(UTIME_OMIT) || \
+   (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && 
__MAC_OS_X_VERSION_MIN_REQUIRED < 1030)
+#define _LIBCPP_HAS_NO_UTIMENSAT
+#endif

dexonsmith wrote:
> EricWF wrote:
> > dexonsmith wrote:
> > > Sadly this isn't quite sufficient.  As per Jack's suggested SDK patch in 
> > > the PR, we need to enumerate the platforms :/.  I think this should be 
> > > the right logic for the four Darwin platforms:
> > > 
> > > (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && 
> > > __IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_11_0) || \
> > > (defined(__WATCH_OS_VERSION_MIN_REQUIRED) &&  
> > > __WATCH_OS_VERSION_MIN_REQUIRED < __WATCHOS_4_0)  || \
> > > (defined(__TV_OS_VERSION_MIN_REQUIRED) && 
> > > __TV_OS_VERSION_MIN_REQUIRED < __TVOS_11_0)   || \
> > > (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && 
> > > __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_13)
> > > 
> > Do we have to do the below dance for all of those macros?
> > 
> > ```
> > #if !defined(__FOO_VERSION_MIN_REQUIRED) && 
> > defined(__ENVIROMENT_FOO_VERSION_MIN_REQUIRED)
> > #define __FOO_VERSION_MIN_REQUIRED __ENVIROMENT_FOO_VERSION_REQUIRED
> > #endif
> > ```
> Nope.  I just advised you to use the wrong ones.  Use the `__ENVIRONMENT` 
> versions pre-defined by Clang.
Specifically:

```
clang/lib/Basic/Targets.cpp:183:  
Builder.defineMacro("__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__", Str);
clang/lib/Basic/Targets.cpp:185:  
Builder.defineMacro("__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__",
clang/lib/Basic/Targets.cpp:197:
Builder.defineMacro("__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__", Str);
clang/lib/Basic/Targets.cpp:221:
Builder.defineMacro("__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__", Str);
```


https://reviews.llvm.org/D34249



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

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: src/experimental/filesystem/operations.cpp:22-24
+#if defined(__APPLE__)
+#include 
+#endif

EricWF wrote:
> dexonsmith wrote:
> > I only just noticed you were including Availability.h.  That shouldn't be 
> > necessary, since the macros should be defined by the compiler.
> __MAC_10_13 et al are defined in `Availability.h`, and 
> `AvailabilityInternal.h` seems to do the `__ENV` dance described above. Are 
> you sure it's not needed?
I don't think the dance is necessary, since libcxx won't be overriding those 
macros.

Also, we can skip the `__MAC_10_13` macros, ala src/chrono.cpp.


https://reviews.llvm.org/D34249



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

[PATCH] D34175: [driver][macOS] Pick the system version for the deployment target if the SDK is newer than the system

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rL LLVM

https://reviews.llvm.org/D34175



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

[PATCH] D33177: any: Add availability for experimental::bad_any_cast

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith added a comment.

Committed in r305647.


https://reviews.llvm.org/D33177



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

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.

Be defensive against a reentrant std::function::operator=(), in case the held 
function object has a non-trivial destructor.  Destroying the function object 
in-place can lead to the destructor being called twice.

rdar://problem/32836603


https://reviews.llvm.org/D34331

Files:
  libcxx/include/functional
  
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
  
libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp

Index: libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/nullptr_t_assign_reentrant.pass.cpp
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(nullptr_t);
+
+#include 
+#include 
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+asm("");
+if (cancel)
+  global = nullptr;
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = nullptr;
+  assert(!A::global.target());
+}
Index: libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/move_reentrant.pass.cpp
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+// class function
+
+// function& operator=(function &&);
+
+#include 
+#include 
+
+struct A
+{
+  static std::function global;
+  static bool cancel;
+
+  ~A() {
+asm("");
+if (cancel)
+  global = std::function(nullptr);
+  }
+  void operator()() {}
+};
+
+std::function A::global;
+bool A::cancel = false;
+
+int main()
+{
+  A::global = A();
+  assert(A::global.target());
+
+  // Check that we don't recurse in A::~A().
+  A::cancel = true;
+  A::global = std::function(nullptr);
+  assert(!A::global.target());
+}
Index: libcxx/include/functional
===
--- libcxx/include/functional
+++ libcxx/include/functional
@@ -1821,11 +1821,7 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(function&& __f) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
-__f_ = 0;
+function::operator=(nullptr);
 if (__f.__f_ == 0)
 __f_ = 0;
 else if ((void *)__f.__f_ == &__f.__buf_)
@@ -1845,11 +1841,12 @@
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(nullptr_t) _NOEXCEPT
 {
-if ((void *)__f_ == &__buf_)
-__f_->destroy();
-else if (__f_)
-__f_->destroy_deallocate();
+__base* __t = __f_;
 __f_ = 0;
+if ((void *)__t == &__buf_)
+__t->destroy();
+else if (__t)
+__t->destroy_deallocate();
 return *this;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34332: path: Use string_view_t consistently

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.

Most of filesystem/path.cpp uses `string_view_t`.  This fixes the two spots 
that use `string_view` directly.


https://reviews.llvm.org/D34332

Files:
  libcxx/src/experimental/filesystem/path.cpp


Index: libcxx/src/experimental/filesystem/path.cpp
===
--- libcxx/src/experimental/filesystem/path.cpp
+++ libcxx/src/experimental/filesystem/path.cpp
@@ -261,7 +261,8 @@
 string_view_pair separate_filename(string_view_t const & s) {
 if (s == "." || s == ".." || s.empty()) return string_view_pair{s, ""};
 auto pos = s.find_last_of('.');
-if (pos == string_view_t::npos) return string_view_pair{s, string_view{}};
+if (pos == string_view_t::npos)
+return string_view_pair{s, string_view_t{}};
 return string_view_pair{s.substr(0, pos), s.substr(pos)};
 }
 
@@ -396,7 +397,7 @@
 size_t hash_value(const path& __p) noexcept {
   auto PP = PathParser::CreateBegin(__p.native());
   size_t hash_value = 0;
-  std::hash hasher;
+  std::hash hasher;
   while (PP) {
 hash_value = __hash_combine(hash_value, hasher(*PP));
 ++PP;


Index: libcxx/src/experimental/filesystem/path.cpp
===
--- libcxx/src/experimental/filesystem/path.cpp
+++ libcxx/src/experimental/filesystem/path.cpp
@@ -261,7 +261,8 @@
 string_view_pair separate_filename(string_view_t const & s) {
 if (s == "." || s == ".." || s.empty()) return string_view_pair{s, ""};
 auto pos = s.find_last_of('.');
-if (pos == string_view_t::npos) return string_view_pair{s, string_view{}};
+if (pos == string_view_t::npos)
+return string_view_pair{s, string_view_t{}};
 return string_view_pair{s.substr(0, pos), s.substr(pos)};
 }
 
@@ -396,7 +397,7 @@
 size_t hash_value(const path& __p) noexcept {
   auto PP = PathParser::CreateBegin(__p.native());
   size_t hash_value = 0;
-  std::hash hasher;
+  std::hash hasher;
   while (PP) {
 hash_value = __hash_combine(hash_value, hasher(*PP));
 ++PP;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34332: path: Use string_view_t consistently

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith added a comment.

Fixed in r305661.


https://reviews.llvm.org/D34332



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

[PATCH] D34251: Add a new driver option to disable warning about c++17's non-throwing exception specification in function signature

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

I think `-Wc++1z-mangling` is best.  IMO, with C++1z in the name, it's already 
clear that this is about compatibility.  I also don't think we need to get as 
specific as `-Wc++1z-compat-exception-spec`.

LGTM with that name, but you might wait a day or two in case someone has a 
strong opinion to the contrary.


https://reviews.llvm.org/D34251



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

[PATCH] D34251: Add a new driver option to disable warning about c++17's non-throwing exception specification in function signature

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

> We use the `-Wc++NN-compat-` prefix on all the other subwarnings of 
> `-Wc++NN-compat` warnings

Interesting; I thought I saw a counter-example recently, and took that as the 
rule (although I can't for the life of me remember what it was).  
`-Wc++NN-compat-mangling` SGTM though.

> Just curious, though: what is the motivation for enabling the rest of 
> `-Wc++1z-compat` but not this part? This seems a lot more serious than the 
> other warnings in the group. (Regardless, I think this patch is useful for 
> the opposite: enabling just this one warning without the rest...)

A user is recompiling the codebase as a unit so binary compatibility doesn't 
matter, but language semantics do.


https://reviews.llvm.org/D34251



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

[PATCH] D34264: Introduce -Wunguarded-availability-new, which is like -Wunguarded-availability, except that it's enabled by default for new deployment targets

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: lib/Sema/SemaDeclAttr.cpp:7031
+Introduced) &&
+!S.Diags.isIgnored(diag::warn_unguarded_availability_new, Loc);
+diag = NewWarning ? diag::warn_partial_availability_new

arphaman wrote:
> erik.pilkington wrote:
> > Sorry to keep this going so long, but why are we even checking isIgnored? 
> > The only difference it could make in whether we emit a diagnostic is if 
> > both: -Wunguarded-availability and -Wno-unguarded-availability-new are 
> > passed in, which seems like it would never happen, right? Even if somebody 
> > did pass that in, it seems reasonable to warn on old stuff but not new 
> > stuff. Maybe I'm missing something here?
> Right, it's to handle the `-Wunguarded-availability 
> -Wno-unguarded-availability-new` case. Your argument makes sense though, we 
> could allow `-Wunguarded-availability -Wno-unguarded-availability-new` where 
> we warn on old APIs. Although that still seems kinda weird to me. Maybe 
> @dexonsmith has an opinion about this?
I don't think the exact behaviour of `-Wunguarded-availability 
-Wno-unguarded-availability-new` is terribly important.  I'm fine either way.

But, having a predictable command-line interface is nice, and I'd expect that 
combination to show diagnostics only for old APIs.


Repository:
  rL LLVM

https://reviews.llvm.org/D34264



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

[PATCH] D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

> I followed the other availability macros in using "strict", but I'm not sure 
> this was the right decision. The documentation seems to recommend not using 
> "strict" (it says that weakly-linking is almost always a better API choice).

libc++ is one of the exceptions.  Use `strict`.


https://reviews.llvm.org/D34556



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

[PATCH] D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: 
test/std/language.support/support.dynamic/new.delete/new.delete.placement/new_deployment.fail.cpp:12-16
+// test availability of new/delete operators introduced in c++17.
+
+#ifdef __APPLE__
+#undef _LIBCPP_DISABLE_AVAILABILITY
+#endif

There is a lit configuration to choose whether to run with availability or not. 
 We should just mark the test unsupported when we don't want it to run.

It's also important to run the rest of the suite with availability turned on, 
to see which other tests start to fail.



Comment at: 
test/std/language.support/support.dynamic/new.delete/new.delete.placement/new_deployment.fail.cpp:21-26
+#ifdef __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
+  int *p0 = new ((std::align_val_t)16) int(1);
+  (void)p0;
+  int *p1 = new ((std::align_val_t)16) int[1];
+  (void)p1;
+#if __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 101300

Similarly here, we can just mark the test unsupported when we don't have 
availability turned on or we're targeting 10.13 or later.


https://reviews.llvm.org/D34556



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

[PATCH] D34578: cmath: Support clang's -fdelayed-template-parsing

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.

r283051 added some functions to cmath (in namespace std) that have the same 
name as functions in math.h (in the global namespace).  Clang's limited support 
for `-fdelayed-template-parsing` chokes on this.  Rename the ones in `cmath` 
and their uses in `complex` and the test.

(Incidentally, I noticed that r283051 was optimizing the implementation of 
``.  I wonder why we have so much code in that header, instead of 
calling out to libc's already-optimized `` implementation?)

rdar://problem/32848355


https://reviews.llvm.org/D34578

Files:
  libcxx/include/cmath
  libcxx/include/complex
  libcxx/test/libcxx/numerics/c.math/constexpr-fns.pass.cpp
  libcxx/test/libcxx/numerics/c.math/fdelayed-template-parsing.sh.cpp
  libcxx/utils/libcxx/test/config.py

Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -440,6 +440,9 @@
 # C++17 aligned allocation.
 self.config.available_features.add('no-aligned-allocation')
 
+if self.cxx.hasCompileFlag('-fdelayed-template-parsing'):
+self.config.available_features.add('fdelayed-template-parsing')
+
 if self.get_lit_bool('has_libatomic', False):
 self.config.available_features.add('libatomic')
 
Index: libcxx/test/libcxx/numerics/c.math/fdelayed-template-parsing.sh.cpp
===
--- /dev/null
+++ libcxx/test/libcxx/numerics/c.math/fdelayed-template-parsing.sh.cpp
@@ -0,0 +1,28 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// test that cmath builds with -fdelayed-template-parsing
+
+// REQUIRES: fdelayed-template-parsing
+
+// RUN: %build -fdelayed-template-parsing
+// RUN: %run
+
+#include 
+#include 
+
+#include "test_macros.h"
+
+int main() {
+  assert(std::isfinite(1.0));
+  assert(!std::isinf(1.0));
+  assert(!std::isnan(1.0));
+}
+
+using namespace std;
Index: libcxx/test/libcxx/numerics/c.math/constexpr-fns.pass.cpp
===
--- libcxx/test/libcxx/numerics/c.math/constexpr-fns.pass.cpp
+++ libcxx/test/libcxx/numerics/c.math/constexpr-fns.pass.cpp
@@ -23,9 +23,9 @@
 
 #include 
 
-static_assert(std::__libcpp_isnan(0.) == false, "");
-static_assert(std::__libcpp_isinf(0.0) == false, "");
-static_assert(std::__libcpp_isfinite(0.0) == true, "");
+static_assert(std::__libcpp_isnan_or_builtin(0.) == false, "");
+static_assert(std::__libcpp_isinf_or_builtin(0.0) == false, "");
+static_assert(std::__libcpp_isfinite_or_builtin(0.0) == true, "");
 
 int main()
 {
Index: libcxx/include/complex
===
--- libcxx/include/complex
+++ libcxx/include/complex
@@ -599,39 +599,39 @@
 _Tp __bc = __b * __c;
 _Tp __x = __ac - __bd;
 _Tp __y = __ad + __bc;
-if (__libcpp_isnan(__x) && __libcpp_isnan(__y))
+if (__libcpp_isnan_or_builtin(__x) && __libcpp_isnan_or_builtin(__y))
 {
 bool __recalc = false;
-if (__libcpp_isinf(__a) || __libcpp_isinf(__b))
+if (__libcpp_isinf_or_builtin(__a) || __libcpp_isinf_or_builtin(__b))
 {
-__a = copysign(__libcpp_isinf(__a) ? _Tp(1) : _Tp(0), __a);
-__b = copysign(__libcpp_isinf(__b) ? _Tp(1) : _Tp(0), __b);
-if (__libcpp_isnan(__c))
+__a = copysign(__libcpp_isinf_or_builtin(__a) ? _Tp(1) : _Tp(0), __a);
+__b = copysign(__libcpp_isinf_or_builtin(__b) ? _Tp(1) : _Tp(0), __b);
+if (__libcpp_isnan_or_builtin(__c))
 __c = copysign(_Tp(0), __c);
-if (__libcpp_isnan(__d))
+if (__libcpp_isnan_or_builtin(__d))
 __d = copysign(_Tp(0), __d);
 __recalc = true;
 }
-if (__libcpp_isinf(__c) || __libcpp_isinf(__d))
+if (__libcpp_isinf_or_builtin(__c) || __libcpp_isinf_or_builtin(__d))
 {
-__c = copysign(__libcpp_isinf(__c) ? _Tp(1) : _Tp(0), __c);
-__d = copysign(__libcpp_isinf(__d) ? _Tp(1) : _Tp(0), __d);
-if (__libcpp_isnan(__a))
+__c = copysign(__libcpp_isinf_or_builtin(__c) ? _Tp(1) : _Tp(0), __c);
+__d = copysign(__libcpp_isinf_or_builtin(__d) ? _Tp(1) : _Tp(0), __d);
+if (__libcpp_isnan_or_builtin(__a))
 __a = copysign(_Tp(0), __a);
-if (__libcpp_isnan(__b))
+if (__libcpp_isnan_or_builtin(__b))
 __b = copysign(_Tp(0), __b);
 __recalc = true;
 }
-if (!__recalc 

[PATCH] D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

LGTM.


https://reviews.llvm.org/D34556



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

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block/function pointer

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D41050#954863, @rjmccall wrote:

> Heh, alright, that works.  It's unfortunate that -disable-llvm-passes doesn't 
> suppress running the pass under -O0; that seems like an oversight.
>
> Anyway, LGTM.


I suspect `-O0` just generates different IR.  You can confirm what passes are 
executed by adding `-mllvm -debug-pass=Executions`.


Repository:
  rC Clang

https://reviews.llvm.org/D41050



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

[PATCH] D41423: [Lex] Avoid out-of-bounds dereference in LexAngledStringLiteral.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Lex/Lexer.cpp:2014-2015
+// getAndAdvanceChar.
+if (C == '\\')
+  C = getAndAdvanceChar(CurPtr, Result);
+

If `CurPtr` is already equal to `BufferEnd`, why is it safe to call 
`getAndAdvanceChar`?  Is `BufferEnd` dereferenceable?



Comment at: clang/lib/Lex/Lexer.cpp:2026
+
+if (C == 0) {
   NulCharacter = CurPtr-1;

Should this check still be skipped (in an `else if` of the `C == '\\'` check)?



Comment at: clang/unittests/Lex/LexerTest.cpp:477
 TEST_F(LexerTest, AvoidPastEndOfStringDereference) {
-  std::vector LexedTokens = Lex("  //  \\\n");
-  EXPECT_TRUE(LexedTokens.empty());
+  EXPECT_TRUE(Lex("  //  \\\n").empty());
+  // rdar://problem/35572754

To minimize the diff, please separate this change into an NFC commit ahead of 
time.



Comment at: clang/unittests/Lex/LexerTest.cpp:478
+  EXPECT_TRUE(Lex("  //  \\\n").empty());
+  // rdar://problem/35572754
+  EXPECT_TRUE(Lex("#include <").empty());

Usually we don't put rdar/bug numbers in the source file.  It would make sense 
in the commit message though.


https://reviews.llvm.org/D41423



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

[PATCH] D41423: [Lex] Avoid out-of-bounds dereference in LexAngledStringLiteral.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/Lex/Lexer.cpp:2026
+
+if (C == 0) {
   NulCharacter = CurPtr-1;

vsapsai wrote:
> dexonsmith wrote:
> > Should this check still be skipped (in an `else if` of the `C == '\\'` 
> > check)?
> I agree it is behaviour change. `NulCharacter` is used to warn if you have 
> null character in the string and I think it is worth warning even if it is 
> preceded by the backslash. Therefore I think this check shouldn't be skipped 
> after `C == '\\'` check. In practice I don't know how you can create a file 
> with null character in its name and use it in inclusion directive.
Can you add a test for the behaviour change then?



Comment at: clang/unittests/Lex/LexerTest.cpp:477
 TEST_F(LexerTest, AvoidPastEndOfStringDereference) {
-  std::vector LexedTokens = Lex("  //  \\\n");
-  EXPECT_TRUE(LexedTokens.empty());
+  EXPECT_TRUE(Lex("  //  \\\n").empty());
+  // rdar://problem/35572754

vsapsai wrote:
> dexonsmith wrote:
> > To minimize the diff, please separate this change into an NFC commit ahead 
> > of time.
> I plan to nominate this fix for inclusion in 6.0.0 release and having one 
> commit will be easier. It is not necessary to include NFC change in 6.0.0 
> release but it creates discrepancy that increases a chance of merge conflicts.
> 
> But I don't have strong opinion, just pointing out potential downsides with 
> merging the change to other branches.
I think it's worth separating the NFC changes from behaviour changes, even if 
it means having to cherry-pick extra patches.


https://reviews.llvm.org/D41423



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

[PATCH] D44263: Implement LWG 2221 - No formatted output operator for nullptr

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

LGTM.


https://reviews.llvm.org/D44263



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: include/clang/Basic/DiagnosticLexKinds.td:713
+def warn_quoted_include_in_framework_header : Warning<
+  "double-quoted include \"%0\" in framework header, expected system style 
 include"
+  >, InGroup, DefaultIgnore;

aaron.ballman wrote:
> bruno wrote:
> > aaron.ballman wrote:
> > > 80-col limit?
> > > 
> > > Also, I'd probably drop "system style" and reword slightly to:
> > > 
> > > `"double-quoted include \"%0\" in framework header, expected 
> > > angle-bracketed include <%0> instead"`
> > Unfortunately this won't work because for framework style includes we use 
> > the angled-bracketed with the framework name. For example, if one wants to 
> > include `Foo.h` from `Foo.framework`, one should use `#include 
> > `, although on disk you actually have 
> > `Foo.framework/Headers/Foo.h`. Framework header lookup does this magic and 
> > other similar ones.
> > 
> > Since we don't know which framework the quoted header could be part of, it 
> > was not included in the warning (doing so would require extra header 
> > searches - which could be expensive for this specific warning). However it 
> > seems that I can do better to indicate that the framework name is desired 
> > here, perhaps:
> > 
> > `"double-quoted include \"%0\" in framework header, expected 
> > angle-bracketed include  instead"`
> > 
> > How does that sound to you? Other suggestions?
> Thank you for the explanation!
> 
> I think your suggested text sounds good, though I do wonder how expensive is 
> "expensive" in finding the intended header? Not only would it provide a 
> better diagnostic, it would also let you use a fixit that doesn't use editor 
> placeholders.
I'm also interested in just how expensive it would be, because I think users 
will be frustrated that the compiler knows it's a framework include "so it 
obviously knows which framework".

I'd be fine if the fix-it came in a follow-up commit though (not sure how Aaron 
feels).



Comment at: lib/Lex/HeaderSearch.cpp:753-754
+  IncluderAndDir.second->getName()))
+Diags.Report(IncludeLoc,
+ diag::warn_quoted_include_in_framework_header)
+<< Filename;

bruno wrote:
> dexonsmith wrote:
> > bruno wrote:
> > > aaron.ballman wrote:
> > > > This seems like a good place for a fix-it to switch the include style. 
> > > > Is there a reason to not do that work for the user?
> > > Like I explained above, we don't know which framework the header could be 
> > > part of, so a fix-it could be misleading.
> > Clang supports editor placeholders, which we use in some refactoring-style 
> > fix-its.  I think this would be spelled `<#framework-name#>`, or `#include 
> > <<#framework-name#>/Foo.h>`
> My current understanding (after chatting with @arphaman) is that editor 
> placeholders isn't a great fit here:
> 
> - For non IDE uses of this, it will just be weird to output something like 
> `#include <<#framework-name#>/Foo.h>`. Even if we wanted to emit this only 
> for IDE use, clang currently has no way to make that distinction (editor 
> placeholder related compiler flags only make sense when actually making the 
> special token sequence lexable, not when generating it)
> - Fixits are (with some known exceptions) meant to be applied to code and 
> subsequently allow compilation to succeed, this wouldn't be the case here.
Okay, sounds good.


Repository:
  rC Clang

https://reviews.llvm.org/D47157



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: include/clang/Basic/DiagnosticLexKinds.td:713
+def warn_quoted_include_in_framework_header : Warning<
+  "double-quoted include \"%0\" in framework header, expected system style 
 include"
+  >, InGroup, DefaultIgnore;

bruno wrote:
> dexonsmith wrote:
> > aaron.ballman wrote:
> > > bruno wrote:
> > > > aaron.ballman wrote:
> > > > > 80-col limit?
> > > > > 
> > > > > Also, I'd probably drop "system style" and reword slightly to:
> > > > > 
> > > > > `"double-quoted include \"%0\" in framework header, expected 
> > > > > angle-bracketed include <%0> instead"`
> > > > Unfortunately this won't work because for framework style includes we 
> > > > use the angled-bracketed with the framework name. For example, if one 
> > > > wants to include `Foo.h` from `Foo.framework`, one should use `#include 
> > > > `, although on disk you actually have 
> > > > `Foo.framework/Headers/Foo.h`. Framework header lookup does this magic 
> > > > and other similar ones.
> > > > 
> > > > Since we don't know which framework the quoted header could be part of, 
> > > > it was not included in the warning (doing so would require extra header 
> > > > searches - which could be expensive for this specific warning). However 
> > > > it seems that I can do better to indicate that the framework name is 
> > > > desired here, perhaps:
> > > > 
> > > > `"double-quoted include \"%0\" in framework header, expected 
> > > > angle-bracketed include  instead"`
> > > > 
> > > > How does that sound to you? Other suggestions?
> > > Thank you for the explanation!
> > > 
> > > I think your suggested text sounds good, though I do wonder how expensive 
> > > is "expensive" in finding the intended header? Not only would it provide 
> > > a better diagnostic, it would also let you use a fixit that doesn't use 
> > > editor placeholders.
> > I'm also interested in just how expensive it would be, because I think 
> > users will be frustrated that the compiler knows it's a framework include 
> > "so it obviously knows which framework".
> > 
> > I'd be fine if the fix-it came in a follow-up commit though (not sure how 
> > Aaron feels).
> I haven't measured, but for each quoted include we would have to:
> 
> - Start a fresh header search.
> - Look for `Foo.h` in all possible frameworks in the path (just on the Darwin 
> macOS SDK path that would be around 140 frameworks).
> - If it's only found in once place, we are mostly safe to say we found a 
> matching framework, otherwise we can't emit a reliable fixit.
> - Header maps and VFS might add extra level of searches (this is very common 
> in Xcode based clang invocations).
Can we just check if it's a header in the *same* framework?


Repository:
  rC Clang

https://reviews.llvm.org/D47157



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: test/Modules/double-quotes.m:24-25
+
+// The same warnings show up when modules is on but -verify doesn't get it
+// because they only show up under the module A building context.
+// RUN: FileCheck --input-file=%t/stderr %s

Would using an explicit module build make this any easier?



Comment at: test/Modules/double-quotes.m:27-29
+// CHECK: double-quoted include "A0.h" in framework header, expected 
angle-bracketed include  instead
+// CHECK: double-quoted include "B.h" in framework header, expected 
angle-bracketed include  instead
+// CHECK: double-quoted include "B.h" in framework header, expected 
angle-bracketed include  instead

When there's a fixit, you don't need to list it in the warning text (the fix-it 
itself is sufficient).  I also feel like "quoted include" is as clear as 
"double-quoted include" (but more succinct).  So I think these would be better 
as:

> warning: quoted include "..." in framework header, expected angle-bracketed 
> include instead



https://reviews.llvm.org/D47157



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

[PATCH] D47157: Warning for framework headers using double quote includes

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: test/Modules/double-quotes.m:27-29
+// CHECK: double-quoted include "A0.h" in framework header, expected 
angle-bracketed include  instead
+// CHECK: double-quoted include "B.h" in framework header, expected 
angle-bracketed include  instead
+// CHECK: double-quoted include "B.h" in framework header, expected 
angle-bracketed include  instead

aaron.ballman wrote:
> dexonsmith wrote:
> > When there's a fixit, you don't need to list it in the warning text (the 
> > fix-it itself is sufficient).  I also feel like "quoted include" is as 
> > clear as "double-quoted include" (but more succinct).  So I think these 
> > would be better as:
> > 
> > > warning: quoted include "..." in framework header, expected 
> > > angle-bracketed include instead
> > 
> Some other lexer diagnostics use "double-quoted" when they want to 
> distinguish with "angle-bracketed" (see 
> `warn_pragma_include_alias_mismatch_angle` and 
> `warn_pragma_include_alias_mismatch_quote` as examples). I don't have a 
> strong opinion on what form we use, but I'd prefer for it to be consistent 
> exposition.
I agree we should be consistent.  No reason to change it here then.


https://reviews.llvm.org/D47157



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

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D47687#1125926, @rnk wrote:

> @dexonsmith is there someone from Apple who can comment on rdar://8678458 and 
> the merits of disabling this warning in macros? I strongly suspect the 
> original report was dealing with code like `assert(x || y && "str");`, if so 
> we can go forward with this.


There were two commits with this radar: r119537 and r119540.  The main 
motivation was a deeply nested macro that when "inlined" included the moral 
equivalent of `#define DOUBLE_OP(OP1, OP2, X, Y, Z) (X OP1 Y OP2 Z)`.  There 
was terrible note spew when the warning fired in this case, and the use case 
for the macro made the warning un-actionable.  We decided to suppress the 
warning entirely:

> As a general comment, the warning seems to be useless for macros; I'll follow 
> the example of warn_logical_instead_of_bitwise which doesn't trigger for 
> macros and I'll make the warning not warn for macros.


https://reviews.llvm.org/D47687



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

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote:

> In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote:
>
> > In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
> >
> > > @dexonsmith is there someone from Apple who can comment on rdar://8678458 
> > > and the merits of disabling this warning in macros? I strongly suspect 
> > > the original report was dealing with code like `assert(x || y && 
> > > "str");`, if so we can go forward with this.
> > >
> > > @chandlerc I know you've hit this behavior difference vs. GCC before. Any 
> > > thoughts on the proposed change?
> >
> >
> >
> >
> > In https://reviews.llvm.org/D47687#1125964, @dexonsmith wrote:
> >
> > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
> > >
> > > > @dexonsmith is there someone from Apple who can comment on 
> > > > rdar://8678458 and the merits of disabling this warning in macros? I 
> > > > strongly suspect the original report was dealing with code like 
> > > > `assert(x || y && "str");`, if so we can go forward with this.
> > >
> > >
> > > There were two commits with this radar: r119537 and r119540.  The main 
> > > motivation was a deeply nested macro that when "inlined" included the 
> > > moral equivalent of `#define DOUBLE_OP(OP1, OP2, X, Y, Z) (X OP1 Y OP2 
> > > Z)`.  There was terrible note spew when the warning fired in this case, 
> > > and the use case for the macro made the warning un-actionable.  We 
> > > decided to suppress the warning entirely:
> > >
> > > > As a general comment, the warning seems to be useless for macros; I'll 
> > > > follow the example of warn_logical_instead_of_bitwise which doesn't 
> > > > trigger for macros and I'll make the warning not warn for macros.
> >
> >
> > Hi, Thank you,
> >
> > I noticed that `warn_logical_instead_of_bitwise ` will also skip 
> > parentheses checking in macros... well, this patch seems not so 
> > necessary... both ok for me ... depends on all of you :-)
>
>
> At worst, we can issue this warning in a new `-Wparentheses-in-macros` 
> subgroup, which, if apple so insists, could be off-by-default.
>  That would less worse than just completely silencing it for the entire world.


I’d be fine with strengthening the existing warning as long as there is an 
actionable fix-it.  I suspect if you suppress it when the relevant expression 
is constructed from multiple macro arguments that will be good enough.


https://reviews.llvm.org/D47687



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

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D47687#1127120, @Higuoxing wrote:

> In https://reviews.llvm.org/D47687#1126607, @dexonsmith wrote:
>
> > In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote:
> >
> > > In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote:
> > >
> > > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
> > > >
> > > > > @dexonsmith is there someone from Apple who can comment on 
> > > > > rdar://8678458 and the merits of disabling this warning in macros? I 
> > > > > strongly suspect the original report was dealing with code like 
> > > > > `assert(x || y && "str");`, if so we can go forward with this.
> > > > >
> > > > > @chandlerc I know you've hit this behavior difference vs. GCC before. 
> > > > > Any thoughts on the proposed change?
> > > >
> > > >
> > > >
> > > >
> > > > In https://reviews.llvm.org/D47687#1125964, @dexonsmith wrote:
> > > >
> > > > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
> > > > >
> > > > > > @dexonsmith is there someone from Apple who can comment on 
> > > > > > rdar://8678458 and the merits of disabling this warning in macros? 
> > > > > > I strongly suspect the original report was dealing with code like 
> > > > > > `assert(x || y && "str");`, if so we can go forward with this.
> > > > >
> > > > >
> > > > > There were two commits with this radar: r119537 and r119540.  The 
> > > > > main motivation was a deeply nested macro that when "inlined" 
> > > > > included the moral equivalent of `#define DOUBLE_OP(OP1, OP2, X, Y, 
> > > > > Z) (X OP1 Y OP2 Z)`.  There was terrible note spew when the warning 
> > > > > fired in this case, and the use case for the macro made the warning 
> > > > > un-actionable.  We decided to suppress the warning entirely:
> > > > >
> > > > > > As a general comment, the warning seems to be useless for macros; 
> > > > > > I'll follow the example of warn_logical_instead_of_bitwise which 
> > > > > > doesn't trigger for macros and I'll make the warning not warn for 
> > > > > > macros.
> > > >
> > > >
> > > > Hi, Thank you,
> > > >
> > > > I noticed that `warn_logical_instead_of_bitwise ` will also skip 
> > > > parentheses checking in macros... well, this patch seems not so 
> > > > necessary... both ok for me ... depends on all of you :-)
> > >
> > >
> > > At worst, we can issue this warning in a new `-Wparentheses-in-macros` 
> > > subgroup, which, if apple so insists, could be off-by-default.
> > >  That would less worse than just completely silencing it for the entire 
> > > world.
> >
> >
> > I’d be fine with strengthening the existing warning as long as there is an 
> > actionable fix-it.  I suspect if you suppress it when the relevant 
> > expression is constructed from multiple macro arguments that will be good 
> > enough.
>
>
> Thanks, currently, `[-Wparentheses | -Wlogical-op-parentheses]` will not emit 
> warning for parentheses in macros. only if you add 
> `[-Wlogical-op-parentheses-in-macros]` it will emit something like `'&&' 
> within '||'` warning...
>
> However, `'&' within '|'` checking was disabled in macros as well... I don't 
> know if this patch meet the needs... if this patch was ok, then, just as 
> @lebedev.ri said, Maybe we could add a `[-Wparentheses-in-macros]` subgroup 
> and add these warning into this new group, in the future... depends on users 
> :-) any suggestion?


Yes, I think understand the patch; but I think it's the wrong direction.  I 
think we should just make the existing `-Wlogical-op-parentheses` smart enough 
to show actionable warnings in macros (but suppress the ones that are not 
actionable, like the internals of `foo(&&, ||, ...)`, rather than adding 
`-Wlogical-op-parentheses-in-macros`, which sounds like it would be permanently 
off-by-default.


https://reviews.llvm.org/D47687



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

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added reviewers: arphaman, ahatanak.
dexonsmith added subscribers: arphaman, ahatanak.
dexonsmith added a comment.

In https://reviews.llvm.org/D47687#1133222, @Higuoxing wrote:

> I think I am almost there.
>
> Here, In my sight
>
>   #define foo(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
>
>
> is un-actionable, because `x op0 y op1 z` are from different arguments of 
> function-like macro, so, we will not check parentheses for op0 or op1 when we 
> call it by
>
>   foo(&&, ||, x, y, z)
>
>
> but if we call it by
>
>   foo(&&, ||, x && y ||z, y, z)
>
>
> it is actionable, because argument (x && y || z) is from the same arg 
> position of macro foo;
>  hence we should check `x && y || z` but shouldn't check parentheses for the 
> first argument (&&) and second argument (||)


SGTM!

> I think this could cover bunch of cases. But I think my code is not so 
> beautiful... So, is there any suggestion?

I made a couple of comments on the tests, but I'd appreciate someone else 
reviewing the code.  @arphaman?  @ahatanak?




Comment at: test/Sema/logical-op-parentheses-in-macros.c:37
+
+void logical_op_from_macro_arguments2(unsigned i) {
+  macro_op_parentheses_check_ops_args(||, &&, i, i, i);  // no 
warning.

Please also check that there's no warning with nested macros:

```
#define VOID(cond) (void)(cond)
#define BUILD_VOID(op1, op2, x, y, z) VOID(x op1 y op2 z)

void foo(unsigned i) { BUILD_VOID(&&, ||, i, i, i); }
```



Comment at: test/Sema/logical-op-parentheses-in-macros.c:52
+}
\ No newline at end of file


Phabricator seems to be reporting that there's a missing newline at the end of 
the file.


https://reviews.llvm.org/D47687



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

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.



Comment at: lib/Sema/SemaExpr.cpp:12304-12309
   // Diagnose "arg1 & arg2 | arg3"
   if ((Opc == BO_Or || Opc == BO_Xor) &&
   !OpLoc.isMacroID()/* Don't warn in macros. */) {
 DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, LHSExpr);
 DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, RHSExpr);
   }

It seems unfortunate not to update this one at the same time.  Or are you 
planning that for a follow-up?

Can you think of a way to share the logic cleanly?



Comment at: lib/Sema/SemaExpr.cpp:12313
   // We don't warn for 'assert(a || b && "bad")' since this is safe.
-  if (Opc == BO_LOr && !OpLoc.isMacroID()/* Don't warn in macros. */) {
-DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
-DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+  if (Opc == BO_LOr) {
+if (!OpLoc.isMacroID()) {

I think the code inside this should be split out into a separate function 
(straw man name: "diagnoseLogicalAndInLogicalOr") so that you can use early 
returns and reduce nesting.



Comment at: lib/Sema/SemaExpr.cpp:12315
+if (!OpLoc.isMacroID()) {
+  // Operator is not in macros
+  DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);

I don't think this comment is valuable.  It's just repeating the code in the 
condition.



Comment at: lib/Sema/SemaExpr.cpp:12319
+} else {
+  // This Expression is expanded from macro
+  SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;

This comment is just repeating what's in the condition.  I suggest replacing it 
with something describing the logic that follows.  Also, it's missing a period 
at the end of the sentence.



Comment at: lib/Sema/SemaExpr.cpp:12321
+  SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
+  if ((Self.getSourceManager().isMacroArgExpansion(OpLoc, 
+  &OpExpansionLoc) &&

This will be cleaner if you create a local reference to 
`Self.getSourceManager()` called `SM`.



Comment at: lib/Sema/SemaExpr.cpp:12325-12328
+  (Self.getSourceManager().isMacroArgExpansion(OpLoc, 
+  &OpExpansionLoc) &&
+  Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+  &LHSExpansionLoc))) {

It's a little awkward to add this condition in with the previous one, and 
you're also repeating a call to `isMacroArgExpansion` unnecessarily.  I suggest 
something like:

```
SourceLocation OpExpansion;
if (!SM.isMacroArgExpansion(OpLoc, &OpExpansion))
  return;

SourceLocation ExprExpansion;
if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansion) &&
OpExpansion == ExprExpansion)
  DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);

if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), &ExprExpansion) &&
OpExpansion == ExprExpansion)
  DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
```




Comment at: test/Sema/parentheses.c:133
+   // expected-note {{place 
parentheses around the '&&' expression to silence this warning}}
+  NESTING_VOID_WRAPPER(&&, ||, ((i && i) || i), i, i); // no waring.
+

typo: should be "no warning".


https://reviews.llvm.org/D47687



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

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:12236
 
+/// Look for '&&' in the righ or left hand of a '||' expr
+static void DiagnoseLogicalAndInLogicalOr(Sema &Self, SourceLocation OpLoc,

- Please add a period at the end of the sentence.
- "righ" should be "right".




Comment at: lib/Sema/SemaExpr.cpp:12243
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+  } else {
+SourceLocation OpExpansionLoc;

You can reduce nesting below by adding an early return above this.

I also think you should describe at a high-level what you're trying to do in 
the code that follows.  Something like:
```
// Only diagnose operators in macros if they're from the same argument position.
```



Comment at: lib/Sema/SemaExpr.cpp:12249
+SourceLocation ExprExpansionLoc;
+// LHS and operator are from same arg position of macro function
+if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansionLoc) &&

This comment is just describing what's clear from the code.  I think you should 
drop it, and the similar one later.



Comment at: lib/Sema/SemaExpr.cpp:12251
+if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansionLoc) &&
+OpExpansionLoc == ExprExpansionLoc)
+  DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);

This line has odd indentation.  Please run clang-format-diff.py to clean up the 
patch.



Comment at: test/Sema/parentheses.c:17-18
 
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )

I don't think these comments are useful.



Comment at: test/Sema/parentheses.c:19
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )

Can you combine this with `NON_NESTING_VOID_0` (which I think should be called 
`VOID_CAST`) below?



Comment at: test/Sema/parentheses.c:20
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+

- You haven't actually wrapped `NESTING_VOID` here.
- A more descriptive name would be `APPLY_OPS` or something.



Comment at: test/Sema/parentheses.c:23
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )

This name is strange.  `VOID_CAST` would be more descriptive.



Comment at: test/Sema/parentheses.c:24
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+

I suggest `APPLY_OPS_DIRECTLY`.



Comment at: test/Sema/parentheses.c:109-111
+  //===--
+  // Logical operator in macros
+  //===--

I'm not sure this comment is particularly useful.



Comment at: test/Sema/parentheses.c:114
+  NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+   // expected-note {{place parentheses around 
the '&&' expression to silence this warning}}
+  NON_NESTING_VOID_0((i && i) || i); // no warning.

Can you add fix-it CHECKs?



Comment at: test/Sema/parentheses.c:117-124
+  // NON_NESTING_VOID_1(&&, ||, i, i, i);
+  // will be expanded to:
+  // i && i || i
+  // ^ arg position 2 (i)
+  //^ arg position 0 (&&)
+  //  ^ arg position 3 (||) should not be checked becaues `op ||` and 
nothing from same arg position
+  // ^ arg position 1 (i)

I think this comment should be fairly well implied by the commit and commit 
message the test is part of.  I don't think it's necessary.



Comment at: test/Sema/parentheses.c:140
+  
+  // same as this one
+  NESTING_VOID_WRAPPER(&&, ||, i && i || i, i, i); // expected-warning {{'&&' 
within '||'}} \

Not a useful comment.



Comment at: test/Sema/parentheses.c:153
+  // ^ arg position 4 (i)
+  NON_NESTING_VOID_1(&&, ||, i, i && i || i, i); // expected-warning {{'&&' 
within '||'}} \
+ // expected-note {{place 
parentheses around the '&&' expression to silence this warning}}

I don't think checking within other macro arguments is necessary here.  You 
have a combinatorial explosion of tests, but it seems unlikely code would be 
written in such a way as to make this wrong.

I would like to see tests like the following:
```
NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning.
NESTI

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Did you miss this comment from my previous review?

> I would like to see tests like the following:
> 
>   NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning.
>   NESTING_VOID_WRAPPER(||, &&, i, i, i || i); // no warning.






Comment at: lib/Sema/SemaExpr.cpp:12254-12255
+if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansionLoc) &&
+!SM.getImmediateMacroCallerLoc(LHSExpr->getExprLoc()).isMacroID() &&
+OpExpansionLoc == ExprExpansionLoc)
+  DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);

Can you reverse these two checks?  The second one looks cheaper.



Comment at: lib/Sema/SemaExpr.cpp:12243
+DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+  } else {
+SourceLocation OpExpansionLoc;

dexonsmith wrote:
> You can reduce nesting below by adding an early return above this.
> 
> I also think you should describe at a high-level what you're trying to do in 
> the code that follows.  Something like:
> ```
> // Only diagnose operators in macros if they're from the same argument 
> position.
> ```
You added an early return -- but then you didn't actually reduce the nesting.  
Please remove the else that follows.



Comment at: test/Sema/parentheses.c:114
+  NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+   // expected-note {{place parentheses around 
the '&&' expression to silence this warning}}
+  NON_NESTING_VOID_0((i && i) || i); // no warning.

Higuoxing wrote:
> Higuoxing wrote:
> > dexonsmith wrote:
> > > Can you add fix-it CHECKs?
> > ```
> > llvm/tools/clang/test/Sema/parentheses.c:109:15: note: place parentheses 
> > around the '&&' expression to silence this warning
> >   VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
> > ~~^~~~
> > llvm/tools/clang/test/Sema/parentheses.c:17:34: note: expanded from macro 
> > 'VOID_CAST'
> > #define VOID_CAST(cond) ( (void)(cond) )
> >  ^~~~
> > ```
> > 
> > Sorry, it seems that when deal with expressions in macros, there is no 
> > fix-it hint ...
> The `suggestParentheses` suppress the fix-it hint when the expression is in 
> macros
> 
> ```
>   if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
>   EndLoc.isValid()) {
> Self.Diag(Loc, Note)
>   << FixItHint::CreateInsertion(ParenRange.getBegin(), "(")
>   << FixItHint::CreateInsertion(EndLoc, ")");
>   } else {
> // We can't display the parentheses, so just show the bare note.
> Self.Diag(Loc, Note) << ParenRange;
>   }
> ```
> 
> You see, there is a `isFileID()`
Can you make it work?  The diagnostic was disabled because it was low quality: 
no fix-it, and firing when it was not actionable.  I'm not convinced we should 
turn it back on until we can give a fix-it.


https://reviews.llvm.org/D47687



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

[PATCH] D53048: [Basic] Split out -Wimplicit-int-conversion and -Wimplicit-float-conversion from -Wconversion

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/test/Sema/implicit-int-conversion.c:1-4
+// RUN: %clang_cc1 -Wconversion -Wno-implicit-int-conversion -DSMALL=char 
-DBIG=int -DNO_DIAG
+// RUN: %clang_cc1 -Wno-conversion -Wimplicit-int-conversion -DSMALL=char 
-DBIG=int
+// RUN: %clang_cc1 -Wconversion -Wno-implicit-float-conversion -DSMALL=float 
-DBIG=double -DNO_DIAG
+// RUN: %clang_cc1 -Wno-conversion -Wimplicit-float-conversion -DSMALL=float 
-DBIG=double

Are these RUN lines missing `-verify`?


Repository:
  rC Clang

https://reviews.llvm.org/D53048



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

[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: lib/Lex/FilterToIncludes.cpp:628
+  First = Id.Last;
+  auto Kind = llvm::StringSwitch(Id.Name)
+  .Case("include", pp_include)

arphaman wrote:
> arphaman wrote:
> > arphaman wrote:
> > > ddunbar wrote:
> > > > What is our feeling w.r.t. _Pragma, which can in theory influence the 
> > > > preprocessor. I'm not sure this model can sanely support it?
> > > We need to look into that. In the worst case we can always avoid 
> > > minimizing the file if it has a _Pragma anywhere in it.
> > We also have to handle cases like this one:
> > 
> > foo.h:
> > ```
> > #define PP _Pragma
> > ```
> > 
> > bar.h:
> > ```
> > PP ("foo")
> > ```
> > 
> > Unfortunately this can't be handled by just disabling minimization for 
> > `foo.h` as PP will be stripped out from `bar.h`. Furthermore, this pattern 
> > is quite reasonable, so we should expect it in the code we receive. Right 
> > now I can't think of a reasonable solution for this problem.
> > 
> > There's also a more "evil" case:
> > 
> > ```
> > #define P(A, B) A##B
> > 
> > P(_Pra,gma)("foo")
> > ```
> > 
> > It would be reasonable to introduce a warning for the use of `_Pragma` 
> > token that was created using PP token concatenation and just hope that our 
> > users won't use this pattern.
> One possible solution to the first issue is to simply fallback to the regular 
> -Eonly invocation if we detect an include of a file that has a macro with a 
> `_Pragma` in it. Then we could emit a remark to the user saying that their 
> build could be faster if they rewrite their code so that this pattern no 
> longer occurs.
Hmm.  Our plan for `@import` is to stop supporting code such as:
```
#define IMPORT(M) @import M
IMPORT(LLVMSupport);
```
where the @import is buried.  I.e., start erroring out in normal Clang builds.  
Perhaps it would be reasonable to similarly disallow `_Pragma` usage that 
buries import/include statements.  I.e., do not support (even in normal builds) 
the following:
```
#define IMPORT(M) _Pragma("clang module import " #M)
IMPORT(LLVMSupport)
```

Possibly, this could be a warning that is error-by-default.


Repository:
  rC Clang

https://reviews.llvm.org/D53354



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

[PATCH] D50539: [VFS] Add property 'fallthrough' that controls fallback to real file system.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In https://reviews.llvm.org/D50539#1273654, @vsapsai wrote:

> I was using `//` instead of `///` on purpose. Class `VFSFromYamlDirIterImpl` 
> resides entirely in .cpp file and isn't available outside of it. Comments are 
> supposed to cover implementation details and intention, not class interface. 
> That's why I think those comments shouldn't be consumed by Doxygen. Does it 
> make sense or do you think it would be better to have those comments in 
> Doxygen?


IMO we should have the doxygen comments for any developer that's trying to 
expand/use/understand the impl class.  In my experience the main reasons we 
stuff things in .cpp files are to optimize compile time and to enable better 
optimizations within a TU.


https://reviews.llvm.org/D50539



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

[PATCH] D50269: [compiler-rt][builtins] Don't #include CoreFoundation in os_version_check.c

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

This LGTM with a couple of nitpicks.




Comment at: compiler-rt/lib/builtins/os_version_check.c:183
   CFStringRef ProductVersion = (*CFStringCreateWithCStringNoCopyFunc)(
-  NULL, "ProductVersion", kCFStringEncodingASCII, kCFAllocatorNull);
+  NULL, "ProductVersion", CF_STRING_ENCODING_ASCII, kCFAllocatorNull);
   if (!ProductVersion)

It wasn't obvious that `kCFAllocatorNull` had been `dlsym`'ed in.  Is there a 
way of naming this that would make it more clear you weren't getting this from 
an `#include`?  Or does it not matter?



Comment at: compiler-rt/lib/builtins/os_version_check.c:211-214
+  if (Major < GlobalMajor) return 1;
+  if (Major > GlobalMajor) return 0;
+  if (Minor < GlobalMinor) return 1;
+  if (Minor > GlobalMinor) return 0;

If you're going to do this, please do it in a separate NFC commit since it 
appears to be whitespace only.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50269



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

[PATCH] D56608: [Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

The code looks good.  Can you add a test too?  Might need to require “shell”.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56608



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

[PATCH] D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D56802#1360316 , @rjmccall wrote:

> We've tossed around the idea of doing things like this before, but I was 
> hoping that it wouldn't have to be specific to `NSObject` and that we'd e.g. 
> have an attribute that guarantees that the `@interface` declares all the 
> ivars for a class.  Are we still thinking that?


The attribute idea hasn't yet survived language discussions (too much of an ABI 
foot-gun for library evolution).  But it's safe to move forward with something 
`NSObject`-specific since it's already baked into the ABI.

> Even if this is just a step towards that, I think we should at least add a 
> method to answer whether we have a statically-known layout for the class, 
> rather than hard-coding that decision at the leaves like this.

A method makes sense to me too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56802



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

[PATCH] D56816: [ObjC] Follow-up r350768 and allow the use of unavailable methods that are declared in a parent class from within the @implementation context

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D56816#1360824 , @smeenai wrote:

> My understanding is that rL349841  
> accidentally started producing some spurious warnings/errors, rL350768 
>  fixed some instances, and this change 
> fixes more instances. Given that the first two changes are already in the 8.0 
> branch, should this be cherry-picked to 8.0 as well once it lands?


Yes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56816



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

[PATCH] D54055: CGDecl::emitStoresForConstant fix synthesized constant's name

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:990-998
+  std::string Name = ("__const." + FunctionName(D.getParentFunctionOrMethod()) 
+
+  "." + D.getName())
+ .str();
+  llvm::GlobalVariable *InsertBefore = nullptr;
+  unsigned AS = CGM.getContext().getTargetAddressSpace(
+  CGM.getStringLiteralAddressSpace());
+  llvm::GlobalVariable *GV = new llvm::GlobalVariable(

Can you just kill `Name` entirely, throwing this inline, to avoid the malloc?


Repository:
  rC Clang

https://reviews.llvm.org/D54055



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

[PATCH] D54630: Move detection of libc++ include dirs to Driver on MacOS

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added 1 blocking reviewer(s): arphaman.
dexonsmith added a comment.

In D54630#1308605 , @arphaman wrote:

> Sounds convincing.
>  @dexonsmith What do you think?


Besides maintaining correct behaviour, I think the most important thing here is 
that the code organization is logical.  Header search is complicated and we 
should be trying to make/keep it simple.  I'm a little skeptical that splitting 
this logic between cc1 and the driver will simplify things, but I haven't 
looked in detail and I'll defer to your (collective) judgement.

> @ilya-biryukov I'm going to do some internal testing to see if we uncover any 
> issues.

I've added you as a blocking reviewer since I think we should work through any 
uncovered issues pre-commit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54630



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

[PATCH] D55455: Remove stat cache chaining as it's no longer needed after PTH support has been removed

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This patch LGTM, but before committing I suggest sending an RFC to cfe-dev and 
waiting a few days.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55455



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

[PATCH] D54604: Automatic variable initialization

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: include/clang/Basic/DiagnosticDriverKinds.td:412
+  "Enable it at your own peril for benchmarking purpose only with "
+  "-enable-trivial-auto-var-init-zero-knowning-it-will-be-removed-from-clang">;
+

s/knowning/knowing/ gives 
`-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang`


Repository:
  rC Clang

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

https://reviews.llvm.org/D54604



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

[PATCH] D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.
dexonsmith added reviewers: bruno, rsmith, vsapsai, Bigcheese.
Herald added subscribers: jdoerfert, jsji, kbarton, mgorny, nemanjai.

Change MemoryBufferCache to InMemoryModuleCache, moving it from Basic to
Serialization.  Another patch will start using it to manage module build
more explicitly, but this is split out because it's mostly mechanical.

Because of the move to Serialization we can no longer abuse the
Preprocessor to forward it to the ASTReader.  Besides the rename and
file move, that means Preprocessor::Preprocessor has one fewer parameter
and ASTReader::ASTReader has one more.


https://reviews.llvm.org/D58890

Files:
  clang/include/clang/Basic/MemoryBufferCache.h
  clang/include/clang/Frontend/ASTUnit.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ASTWriter.h
  clang/include/clang/Serialization/InMemoryModuleCache.h
  clang/include/clang/Serialization/Module.h
  clang/include/clang/Serialization/ModuleManager.h
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/MemoryBufferCache.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/ChainedIncludesSource.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Frontend/Rewrite/FrontendActions.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/CMakeLists.txt
  clang/lib/Serialization/GeneratePCH.cpp
  clang/lib/Serialization/InMemoryModuleCache.cpp
  clang/lib/Serialization/ModuleManager.cpp
  clang/test/Modules/outofdate-rebuild.m
  clang/unittests/Basic/CMakeLists.txt
  clang/unittests/Basic/MemoryBufferCacheTest.cpp
  clang/unittests/Basic/SourceManagerTest.cpp
  clang/unittests/CMakeLists.txt
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Lex/LexerTest.cpp
  clang/unittests/Lex/PPCallbacksTest.cpp
  clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
  clang/unittests/Serialization/CMakeLists.txt
  clang/unittests/Serialization/InMemoryModuleCacheTest.cpp

Index: clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
===
--- clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
+++ clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
@@ -1,4 +1,4 @@
-//===- MemoryBufferCacheTest.cpp - MemoryBufferCache tests ===//
+//===- InMemoryModuleCacheTest.cpp - InMemoryModuleCache tests ===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,7 +6,7 @@
 //
 //===--===//
 
-#include "clang/Basic/MemoryBufferCache.h"
+#include "clang/Serialization/InMemoryModuleCache.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "gtest/gtest.h"
 
@@ -22,7 +22,7 @@
 /* RequiresNullTerminator = */ false);
 }
 
-TEST(MemoryBufferCacheTest, addBuffer) {
+TEST(InMemoryModuleCacheTest, addBuffer) {
   auto B1 = getBuffer(1);
   auto B2 = getBuffer(2);
   auto B3 = getBuffer(3);
@@ -31,7 +31,7 @@
   auto *RawB3 = B3.get();
 
   // Add a few buffers.
-  MemoryBufferCache Cache;
+  InMemoryModuleCache Cache;
   EXPECT_EQ(RawB1, &Cache.addBuffer("1", std::move(B1)));
   EXPECT_EQ(RawB2, &Cache.addBuffer("2", std::move(B2)));
   EXPECT_EQ(RawB3, &Cache.addBuffer("3", std::move(B3)));
@@ -58,9 +58,9 @@
   EXPECT_FALSE(Cache.isBufferFinal("3"));
 }
 
-TEST(MemoryBufferCacheTest, finalizeCurrentBuffers) {
+TEST(InMemoryModuleCacheTest, finalizeCurrentBuffers) {
   // Add a buffer.
-  MemoryBufferCache Cache;
+  InMemoryModuleCache Cache;
   auto B1 = getBuffer(1);
   auto *RawB1 = B1.get();
   Cache.addBuffer("1", std::move(B1));
Index: clang/unittests/Serialization/CMakeLists.txt
===
--- /dev/null
+++ clang/unittests/Serialization/CMakeLists.txt
@@ -0,0 +1,17 @@
+set(LLVM_LINK_COMPONENTS
+  BitReader
+  Support
+  )
+
+add_clang_unittest(SerializationTests
+  InMemoryModuleCacheTest.cpp
+  )
+
+target_link_libraries(SerializationTests
+  PRIVATE
+  clangAST
+  clangBasic
+  clangLex
+  clangSema
+  clangSerialization
+  )
Index: clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
===
--- clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
+++ clang/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
@@ -11,7 +11,6 @@
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/LangOptions.h"
-#include "clang/Basic/MemoryBufferCache.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic

[PATCH] D58891: Modules: Add -Rmodule-import

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.
dexonsmith added reviewers: Bigcheese, bruno, rsmith, vsapsai.
Herald added a subscriber: jdoerfert.

Add a remark for importing modules.  Depending on whether this is a
direct import (into the TU being built by this compiler instance) or
transitive import (into an already-imported module), the diagnostic has
two forms:

  importing module 'Foo' from 'path/to/Foo.pcm'
  importing module 'Foo' into 'Bar' from 'path/to/Foo.pcm'

Also drop a redundant FileCheck invocation in Rmodule-build.m that was
using -Reverything, since the notes from -Rmodule-import were confusing
it.


https://reviews.llvm.org/D58891

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSerializationKinds.td
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Modules/Inputs/Rmodule-import/A.h
  clang/test/Modules/Inputs/Rmodule-import/B.h
  clang/test/Modules/Inputs/Rmodule-import/C.h
  clang/test/Modules/Inputs/Rmodule-import/D.h
  clang/test/Modules/Inputs/Rmodule-import/module.modulemap
  clang/test/Modules/Rmodule-build.m
  clang/test/Modules/Rmodule-import.m

Index: clang/test/Modules/Rmodule-import.m
===
--- /dev/null
+++ clang/test/Modules/Rmodule-import.m
@@ -0,0 +1,46 @@
+// RUN: rm -rf %t1 %t2
+
+// Run with -verify, which onliy gets remarks from the main TU.
+//
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t1 \
+// RUN: -fdisable-module-hash -fsyntax-only -I%S/Inputs/Rmodule-import \
+// RUN: -Rmodule-build -Rmodule-import -verify %s
+
+// Run again, using FileCheck to check remarks from the module builds.
+//
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t2 \
+// RUN: -fdisable-module-hash -fsyntax-only -I%S/Inputs/Rmodule-import \
+// RUN: -Rmodule-build -Rmodule-import %s 2>&1 |\
+// RUN: FileCheck %s -implicit-check-not "remark:"
+
+#include "A.h" // \
+   expected-remark-re{{building module 'A' as '{{.*}}/A.pcm'}} \
+   expected-remark{{finished building module 'A'}} \
+   expected-remark-re{{importing module 'A' from '{{.*}}/A.pcm'}} \
+   expected-remark-re{{importing module 'B' into 'A' from '{{.*}}/B.pcm'}} \
+   expected-remark-re{{importing module 'C' into 'B' from '{{.*}}/C.pcm'}}
+// CHECK: remark: building module 'A'
+// CHECK: remark: building module 'B'
+// CHECK: remark: building module 'C'
+// CHECK: remark: finished building module 'C'
+// CHECK: remark: importing module 'C' from '{{.*}}/C.pcm'
+// CHECK: remark: finished building module 'B'
+// CHECK: remark: importing module 'B' from '{{.*}}/B.pcm'
+// CHECK: remark: importing module 'C' into 'B' from '{{.*}}/C.pcm'
+// CHECK: remark: finished building module 'A'
+// CHECK: remark: importing module 'A' from '{{.*}}/A.pcm'
+// CHECK: remark: importing module 'B' into 'A' from '{{.*}}/B.pcm'
+// CHECK: remark: importing module 'C' into 'B' from '{{.*}}/C.pcm'
+#include "B.h" // \
+   expected-remark-re{{importing module 'B' from '{{.*}}/B.pcm'}}
+// CHECK: remark: importing module 'B' from '{{.*}}/B.pcm'
+#include "C.h" // \
+   expected-remark-re{{importing module 'C' from '{{.*}}/C.pcm'}}
+// CHECK: remark: importing module 'C' from '{{.*}}/C.pcm'
+#include "D.h" // \
+   expected-remark-re{{building module 'D' as '{{.*}}/D.pcm'}} \
+   expected-remark{{finished building module 'D'}} \
+   expected-remark-re{{importing module 'D' from '{{.*}}/D.pcm'}}
+// CHECK: remark: building module 'D'
+// CHECK: remark: finished building module 'D'
+// CHECK: remark: importing module 'D' from '{{.*}}/D.pcm'
Index: clang/test/Modules/Rmodule-build.m
===
--- clang/test/Modules/Rmodule-build.m
+++ clang/test/Modules/Rmodule-build.m
@@ -19,10 +19,6 @@
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fsyntax-only %s -I %t \
 // RUN:-Rmodule-build 2>&1 | FileCheck %s
 
-// RUN: echo ' ' >> %t/C.h
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fsyntax-only %s -I %t \
-// RUN:-Reverything 2>&1 | FileCheck %s
-
 // RUN: echo ' ' >> %t/B.h
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fsyntax-only %s -I %t \
 // RUN:2>&1 | FileCheck -allow-empty -check-prefix=NO-REMARKS %s
Index: clang/test/Modules/Inputs/Rmodule-import/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/Rmodule-import/module.modulemap
@@ -0,0 +1,4 @@
+module A { header "A.h" }
+module B { header "B.h" }
+module C { header "C.h" }
+module D { header "D.h" }
Index: clang/test/Modules/Inputs/Rmodule-import/D.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/Rmodule-import/D.h
@@ -0,0 +1 @@
+// D
Index: clang/test/Modules/Inputs/Rmodule-import/C.h

[PATCH] D58893: Modules: Invalidate out-of-date PCMs as they're discovered

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.
dexonsmith added reviewers: Bigcheese, bruno, rsmith, vsapsai.
Herald added a subscriber: jdoerfert.
dexonsmith added parent revisions: D58890: Modules: Rename MemoryBufferCache to 
InMemoryModuleCache, D58891: Modules: Add -Rmodule-import.

Leverage the InMemoryModuleCache to invalidate a module the first time
it fails to import (and to lock a module as soon as it's built or
imported successfully).  For implicit module builds, this optimizes
importing deep graphs where the leaf module is out-of-date; see example
near the end of the commit message.

Previously the cache finalized ("locked in") all modules imported so far
when starting a new module build.  This was sufficient to prevent
loading two versions of the same module, but was somewhat arbitrary and
hard to reason about.

Now the cache explicitly tracks module state, where each module must be
one of:

- Unknown: module not in the cache (yet).
- Tentative: module in the cache, but not yet fully imported.
- ToBuild: module found on disk could not be imported; need to build.
- Final: module in the cache has been successfully built or imported.

Preventing repeated failed imports avoids variation in builds based on
shifting filesystem state.  Now it's guaranteed that a module is loaded
from disk exactly once.  It now seems safe to remove
FileManager::invalidateCache, but I'm leaving that for a later commit.

The new, precise logic uncovered a pre-existing problem in the cache:
the map key is the module filename, and different contexts use different
filenames for the same PCM file.  (In particular, the test
Modules/relative-import-path.c does not build without this commit.
r223577 started using a relative path to describe a module's base
directory when importing it within another module.  As a result, the
module cache sees an absolute path when (a) building the module or
importing it at the top-level, and a relative path when (b) importing
the module underneath another one.)

The "obvious" fix is to resolve paths using FileManager::getVirtualFile
and change the map key for the cache to a FileEntry, but some contexts
(particularly related to ASTUnit) have a shorter lifetime for their
FileManager than the InMemoryModuleCache.  This is worth pursuing
further in a later commit; perhaps by tying together the FileManager and
InMemoryModuleCache lifetime, or moving the in-memory PCM storage into a
VFS layer.

For now, use the PCM's base directory as-written for constructing the
filename to check the ModuleCache.

Example
===

To understand the build optimization, first consider the build of a
module graph TU -> A -> B -> C -> D with an empty cache:

  TU builds A'
 A' builds B'
B' builds C'
   C' builds D'
  imports D'
B' imports C'
   imports D'
 A' imports B'
imports C'
imports D'
  TU imports A'
 imports B'
 imports C'
 imports D'

If we build TU again, where A, B, C, and D are in the cache and D is
out-of-date, we would previously get this build:

  TU imports A
 imports B
 imports C
 imports D (out-of-date)
  TU builds A'
 A' imports B
imports C
imports D (out-of-date)
builds B'
B' imports C
   imports D (out-of-date)
   builds C'
   C' imports D (out-of-date)
  builds D'
  imports D'
B' imports C'
   imports D'
 A' imports B'
imports C'
imports D'
   TU imports A'
  imports B'
  imports C'
  imports D'

After this commit, we'll immediateley invalidate A, B, C, and D when we
first observe that D is out-of-date, giving this build:

  TU imports A
 imports B
 imports C
 imports D (out-of-date)
  TU builds A' // The same graph as an empty cache.
 A' builds B'
B' builds C'
   C' builds D'
  imports D'
B' imports C'
   imports D'
 A' imports B'
imports C'
imports D'
  TU imports A'
 imports B'
 imports C'
 imports D'

The new build matches what we'd naively expect, pretty closely matching
the original build with the empty cache.

rdar://problem/48545366


https://reviews.llvm.org/D58893

Files:
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/InMemoryModuleCache.h
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/InMemoryModuleCache.cpp
  clang/lib/Serialization/ModuleManager.cpp
  clang/test/Modules/Inputs/implicit-invalidate-chain/A.h
  clang/test/Modules/Inputs/implicit-invalidate-chain/B.h
  clang/test/Modules/Inputs/implicit-invalidate-chain/C.h
  clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap
  clang/test/Modules/Inputs/relative-import-path/A.h
  clang/test/Modules/Inputs/relative-import-path/B.h
  clang/test/Modules/Inputs/relative-import-path/C.h
  clang

[PATCH] D58891: Modules: Add -Rmodule-import

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith marked an inline comment as done.
dexonsmith added inline comments.



Comment at: clang/test/Modules/Rmodule-build.m:22-25
-// RUN: echo ' ' >> %t/C.h
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t 
-fsyntax-only %s -I %t \
-// RUN:-Reverything 2>&1 | FileCheck %s
-

@bruno this looked redundant to me with the check above; can you confirm?  (Now 
that we have `-Rmodule-import`, the FileCheck was failing...)


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

https://reviews.llvm.org/D58891



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

[PATCH] D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D58890#1417465 , @bruno wrote:

> `InMemoryModuleCache` seems like a way more appropriate name here. Also 
> thanks for improving some of the comments.
>
> > Because of the move to Serialization we can no longer abuse the 
> > Preprocessor to forward it to the ASTReader. Besides the rename and file 
> > move, that means Preprocessor::Preprocessor has one fewer parameter and 
> > ASTReader::ASTReader has one more.
>
> Are there any C api bits that expose preprocessor stuff and also need updates?


`ninja check-clang` passes... is there anything else I should be testing?


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

https://reviews.llvm.org/D58890



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

[PATCH] D58891: Modules: Add -Rmodule-import

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith marked 2 inline comments as done.
dexonsmith added a comment.

Committed in r355477.




Comment at: clang/test/Modules/Inputs/Rmodule-import/B.h:1
+// B
+#include "C.h"

bruno wrote:
> Can you make one of the tests to use `@import` (or the pragma version) 
> instead of #include? 
I changed the import of `D`.


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

https://reviews.llvm.org/D58891



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

[PATCH] D59032: Passthrough compiler launcher

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

LGTM.  Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59032



___
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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.
dexonsmith added reviewers: bruno, rsmith, vsapsai, jordan_rose.
Herald added a subscriber: jdoerfert.

Add an option to cache the generated PCH in the ModuleCache when
emitting it.  This protects clients that build PCHs and read them in the
same process, allowing them to avoid race conditions between parallel
jobs the same way that Clang's implicit module build system does.


https://reviews.llvm.org/D59176

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GeneratePCH.cpp
  clang/unittests/Frontend/FrontendActionTest.cpp

Index: clang/unittests/Frontend/FrontendActionTest.cpp
===
--- clang/unittests/Frontend/FrontendActionTest.cpp
+++ clang/unittests/Frontend/FrontendActionTest.cpp
@@ -6,18 +6,20 @@
 //
 //===--===//
 
+#include "clang/Frontend/FrontendAction.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
-#include "clang/Frontend/FrontendAction.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/Sema.h"
+#include "clang/Serialization/InMemoryModuleCache.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/ToolOutputFile.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
@@ -253,4 +255,40 @@
   EXPECT_EQ("This is a note", TDC->Note.str().str());
 }
 
+TEST(GeneratePCHFrontendAction, CacheGeneratedPCH) {
+  // Create a temporary file for writing out the PCH that will be cleaned up.
+  int PCHFD;
+  llvm::SmallString<128> PCHFilename;
+  ASSERT_FALSE(
+  llvm::sys::fs::createTemporaryFile("test.h", "pch", PCHFD, PCHFilename));
+  llvm::ToolOutputFile PCHFile(PCHFilename, PCHFD);
+
+  for (bool ShouldCache : {false, true}) {
+auto Invocation = std::make_shared();
+Invocation->getLangOpts()->CacheGeneratedPCH = ShouldCache;
+Invocation->getPreprocessorOpts().addRemappedFile(
+"test.h",
+MemoryBuffer::getMemBuffer("int foo(void) { return 1; }\n").release());
+Invocation->getFrontendOpts().Inputs.push_back(
+FrontendInputFile("test.h", InputKind::C));
+Invocation->getFrontendOpts().OutputFile = StringRef(PCHFilename);
+Invocation->getFrontendOpts().ProgramAction = frontend::GeneratePCH;
+Invocation->getTargetOpts().Triple = "x86_64-apple-darwin19.0.0";
+CompilerInstance Compiler;
+Compiler.setInvocation(std::move(Invocation));
+Compiler.createDiagnostics();
+
+GeneratePCHAction TestAction;
+ASSERT_TRUE(Compiler.ExecuteAction(TestAction));
+
+// Check whether the PCH was cached.
+if (ShouldCache)
+  EXPECT_EQ(InMemoryModuleCache::Final,
+Compiler.getModuleCache().getPCMState(PCHFilename));
+else
+  EXPECT_EQ(InMemoryModuleCache::Unknown,
+Compiler.getModuleCache().getPCMState(PCHFilename));
+  }
+}
+
 } // anonymous namespace
Index: clang/lib/Serialization/GeneratePCH.cpp
===
--- clang/lib/Serialization/GeneratePCH.cpp
+++ clang/lib/Serialization/GeneratePCH.cpp
@@ -24,12 +24,14 @@
 const Preprocessor &PP, InMemoryModuleCache &ModuleCache,
 StringRef OutputFile, StringRef isysroot, std::shared_ptr Buffer,
 ArrayRef> Extensions,
-bool AllowASTWithErrors, bool IncludeTimestamps)
+bool AllowASTWithErrors, bool IncludeTimestamps,
+bool ShouldCacheASTInMemory)
 : PP(PP), OutputFile(OutputFile), isysroot(isysroot.str()),
   SemaPtr(nullptr), Buffer(std::move(Buffer)), Stream(this->Buffer->Data),
   Writer(Stream, this->Buffer->Data, ModuleCache, Extensions,
  IncludeTimestamps),
-  AllowASTWithErrors(AllowASTWithErrors) {
+  AllowASTWithErrors(AllowASTWithErrors),
+  ShouldCacheASTInMemory(ShouldCacheASTInMemory) {
   this->Buffer->IsComplete = false;
 }
 
@@ -61,7 +63,8 @@
   Writer.WriteAST(*SemaPtr, OutputFile, Module, isysroot,
   // For serialization we are lenient if the errors were
   // only warn-as-error kind.
-  PP.getDiagnostics().hasUncompilableErrorOccurred());
+  PP.getDiagnostics().hasUncompilableErrorOccurred(),
+  ShouldCacheASTInMemory);
 
   Buffer->IsComplete = true;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4595,7 +4595,8 @@
 ASTFileSignature ASTWriter::WriteAST(Sema 

[PATCH] D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith added a comment.

Committed in r355777.


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

https://reviews.llvm.org/D58890



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

[PATCH] D58893: Modules: Invalidate out-of-date PCMs as they're discovered

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith marked 2 inline comments as done.
dexonsmith added a comment.

Committed in r355778.




Comment at: clang/include/clang/Serialization/InMemoryModuleCache.h:37
 
 /// Track the timeline of when this was added to the cache.
+bool IsFinal = false;

bruno wrote:
> Now that generation count is gone, update (or remove) this comment?
Nice catch.  Updated in final patch.


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

https://reviews.llvm.org/D58893



___
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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith marked 2 inline comments as done.
dexonsmith added a comment.

In D59176#1424864 , @jordan_rose wrote:

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


Correct, it just exposes an option.




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

jordan_rose wrote:
> What's the `+` for?
Heh, I was trying to figure out those `+`s as well until I added this without 
one and it didn't compile.  The problem is that bitfields can't be forwarded 
through the `llvm::make_unique` variadic.  The `+` creates a local temporary 
with the same value.



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

jordan_rose wrote:
> 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`.)
> (Note that BuildingImplicitModule probably isn't the same as the original 
> condition ImplicitModules.)

Nice catch; I ported the logic from `ASTWriter` incorrectly.  I'll fix this.

> 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()?

I was cargo-culting the logic for how `PCHGenerator::HandleTranslationUnit` 
sets the `Module` parameter to `ASTWriter::WriteAST`.  I think that if I fix 
the above to match the original condition I'll need to check 
`isCompilingModule()`... but maybe `BuildingImplicitModule` is already `&&`-ing 
these together?  I'll check.


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] D59176: Modules: Add LangOptions::CacheGeneratedPCH

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith updated this revision to Diff 190203.
dexonsmith added a comment.

Updated the constructor call to `PCHGenerator` in 
`GenerateModuleAction::CreateASTConsumer` to use `BuildingImplicitModule` on 
its own.  Checking where it's set (only in `compileModuleImpl`), it's exactly 
the condition we want here.


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

https://reviews.llvm.org/D59176

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Serialization/ASTWriter.h
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/GeneratePCH.cpp
  clang/unittests/Frontend/FrontendActionTest.cpp

Index: clang/unittests/Frontend/FrontendActionTest.cpp
===
--- clang/unittests/Frontend/FrontendActionTest.cpp
+++ clang/unittests/Frontend/FrontendActionTest.cpp
@@ -6,18 +6,20 @@
 //
 //===--===//
 
+#include "clang/Frontend/FrontendAction.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
-#include "clang/Frontend/FrontendAction.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/Sema.h"
+#include "clang/Serialization/InMemoryModuleCache.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/ToolOutputFile.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
@@ -253,4 +255,40 @@
   EXPECT_EQ("This is a note", TDC->Note.str().str());
 }
 
+TEST(GeneratePCHFrontendAction, CacheGeneratedPCH) {
+  // Create a temporary file for writing out the PCH that will be cleaned up.
+  int PCHFD;
+  llvm::SmallString<128> PCHFilename;
+  ASSERT_FALSE(
+  llvm::sys::fs::createTemporaryFile("test.h", "pch", PCHFD, PCHFilename));
+  llvm::ToolOutputFile PCHFile(PCHFilename, PCHFD);
+
+  for (bool ShouldCache : {false, true}) {
+auto Invocation = std::make_shared();
+Invocation->getLangOpts()->CacheGeneratedPCH = ShouldCache;
+Invocation->getPreprocessorOpts().addRemappedFile(
+"test.h",
+MemoryBuffer::getMemBuffer("int foo(void) { return 1; }\n").release());
+Invocation->getFrontendOpts().Inputs.push_back(
+FrontendInputFile("test.h", InputKind::C));
+Invocation->getFrontendOpts().OutputFile = StringRef(PCHFilename);
+Invocation->getFrontendOpts().ProgramAction = frontend::GeneratePCH;
+Invocation->getTargetOpts().Triple = "x86_64-apple-darwin19.0.0";
+CompilerInstance Compiler;
+Compiler.setInvocation(std::move(Invocation));
+Compiler.createDiagnostics();
+
+GeneratePCHAction TestAction;
+ASSERT_TRUE(Compiler.ExecuteAction(TestAction));
+
+// Check whether the PCH was cached.
+if (ShouldCache)
+  EXPECT_EQ(InMemoryModuleCache::Final,
+Compiler.getModuleCache().getPCMState(PCHFilename));
+else
+  EXPECT_EQ(InMemoryModuleCache::Unknown,
+Compiler.getModuleCache().getPCMState(PCHFilename));
+  }
+}
+
 } // anonymous namespace
Index: clang/lib/Serialization/GeneratePCH.cpp
===
--- clang/lib/Serialization/GeneratePCH.cpp
+++ clang/lib/Serialization/GeneratePCH.cpp
@@ -24,12 +24,14 @@
 const Preprocessor &PP, InMemoryModuleCache &ModuleCache,
 StringRef OutputFile, StringRef isysroot, std::shared_ptr Buffer,
 ArrayRef> Extensions,
-bool AllowASTWithErrors, bool IncludeTimestamps)
+bool AllowASTWithErrors, bool IncludeTimestamps,
+bool ShouldCacheASTInMemory)
 : PP(PP), OutputFile(OutputFile), isysroot(isysroot.str()),
   SemaPtr(nullptr), Buffer(std::move(Buffer)), Stream(this->Buffer->Data),
   Writer(Stream, this->Buffer->Data, ModuleCache, Extensions,
  IncludeTimestamps),
-  AllowASTWithErrors(AllowASTWithErrors) {
+  AllowASTWithErrors(AllowASTWithErrors),
+  ShouldCacheASTInMemory(ShouldCacheASTInMemory) {
   this->Buffer->IsComplete = false;
 }
 
@@ -61,7 +63,8 @@
   Writer.WriteAST(*SemaPtr, OutputFile, Module, isysroot,
   // For serialization we are lenient if the errors were
   // only warn-as-error kind.
-  PP.getDiagnostics().hasUncompilableErrorOccurred());
+  PP.getDiagnostics().hasUncompilableErrorOccurred(),
+  ShouldCacheASTInMemory);
 
   Buffer->IsComplete = true;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4596,7 +4596,8 @@
 ASTFileSignature ASTWriter::WriteAST(Sema &SemaRef,
  

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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith marked 2 inline comments as done.
dexonsmith added inline comments.



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

dexonsmith wrote:
> jordan_rose wrote:
> > 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`.)
> > (Note that BuildingImplicitModule probably isn't the same as the original 
> > condition ImplicitModules.)
> 
> Nice catch; I ported the logic from `ASTWriter` incorrectly.  I'll fix this.
> 
> > 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()?
> 
> I was cargo-culting the logic for how `PCHGenerator::HandleTranslationUnit` 
> sets the `Module` parameter to `ASTWriter::WriteAST`.  I think that if I fix 
> the above to match the original condition I'll need to check 
> `isCompilingModule()`... but maybe `BuildingImplicitModule` is already 
> `&&`-ing these together?  I'll check.
Updated the patch to just use `BuildingImplicitModule`.  The previous condition 
in `PCHGenerator::HandleTranslationUnit` seems to have been equivalent.

- Previously in `PCHGenerator::HandleTranslationUnit`, `WritingModule` would be 
non-null only if we're currently building a module, as opposed to writing out a 
PCH.  The logic to write to the in-memory cache also checked if 
`LangOptions::ImplicitModules` was set.
- Now in `GenerateModuleAction::CreateASTConsumer` we check 
`BuildingImplicitModule` which is set in `compileModuleImpl` before executing 
the action.

I'm choosing this because it better matches the code around.


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] D59176: Modules: Add LangOptions::CacheGeneratedPCH

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith added a comment.

Committed in r355950.  Thanks for the review.


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] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D58514#1428434 , @ahatanak wrote:

> Seems like the chromium code is valid and shouldn't crash. John/Erik what do 
> you think? The following code also crashes with this patch applied.
>
>   typedef void (^BlockTy)();
>  
>   BlockTy sb;
>   __weak BlockTy wb;
>  
>   void foo(id a) {
> auto b = ^{ NSLog(@"foo %@", a); };
> wb = b; // block isn't copied to the heap.
> sb = b; // block is copied to the heap.
>   }
>  
>   int main() {
> auto x = [NSObject new];
> foo(x);
> sb();
> wb();
> return 0;
>   }
>


The assignment to `wb` seems like an escape of some sort.  What happens for 
this similar code?

  typedef void (^BlockTy)();
  
  BlockTy sb;
  __weak BlockTy wb;
  
  void bar(id b) {
wb = b;
sb = b;
  }
  
  void foo(id a) {
bar(^{ NSLog(@"foo %@", a); });
  }
  
  int main() {
auto x = [NSObject new];
foo(x);
sb();
wb();
return 0;
  }


Repository:
  rC Clang

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

https://reviews.llvm.org/D58514



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

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D58514#1428520 , @ahatanak wrote:

> That code doesn't crash because the block is retained at the entry of `bar` 
> and ARC optimizer doesn't remove the retain/release pairs in `bar`.


Oops, I meant:

  typedef void (^BlockTy)();
  
  BlockTy sb;
  __weak BlockTy wb;
  
  void bar(BlockTy b) {
wb = b;
sb = b;
  }
  
  void foo(id a) {
bar(^{ NSLog(@"foo %@", a); });
  }
  
  int main() {
auto x = [NSObject new];
foo(x);
sb();
wb();
return 0;
  }

Is it the same?  Does `b` get retained at the entry to `bar()`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58514



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

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D58514#1428567 , @ahatanak wrote:

> Sorry, I misread the code. If you change the parameter type of `bar` to 
> `BlockTy`, the code crashes. If the type is `id`, it doesn't crash because 
> IRGen copies the block to the heap in `foo` before passing it to `bar`.


Okay, that's what I expected.  (You didn't misread, I had just mistyped :/.)

How insane would it be to add a copy/dispose pair around assignments of blocks 
to weak references?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58514



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

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.
Herald added subscribers: jdoerfert, kadircet, arphaman, jkorous.

Remove CompilerInstance::VirtualFileSystem and
CompilerInstance::setVirtualFileSystem, instead relying on the VFS in
the FileManager.  CompilerInstance and its clients already went to some
trouble to make these match.  Now they are guaranteed to match.

As part of this, I added a VFS parameter (defaults to nullptr) to
CompilerInstance::createFileManager to avoid repeating construction
logic in clients that just want to customize the VFS.


https://reviews.llvm.org/D59377

Files:
  clang-tools-extra/clangd/Compiler.cpp
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -671,7 +671,7 @@
   }
 
   // Make sure clang uses the same VFS as LLDB.
-  instance->setVirtualFileSystem(FileSystem::Instance().GetVirtualFileSystem());
+  instance->createFileManager(FileSystem::Instance().GetVirtualFileSystem());
   instance->setDiagnostics(diagnostics_engine.get());
   instance->setInvocation(invocation);
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -309,8 +309,7 @@
   }
 
   // Make sure clang uses the same VFS as LLDB.
-  m_compiler->setVirtualFileSystem(
-  FileSystem::Instance().GetVirtualFileSystem());
+  m_compiler->createFileManager(FileSystem::Instance().GetVirtualFileSystem());
 
   lldb::LanguageType frame_lang =
   expr.Language(); // defaults to lldb::eLanguageTypeUnknown
Index: clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
+++ clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
@@ -82,8 +82,6 @@
 
   Instance.getDiagnostics().setSourceManager(&SM);
 
-  Instance.setVirtualFileSystem(&CI.getVirtualFileSystem());
-
   // The instance wants to take ownership, however DisableFree frontend option
   // is set to true to avoid double free issues
   Instance.setFileManager(&CI.getFileManager());
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -89,10 +89,6 @@
 
 void CompilerInstance::setFileManager(FileManager *Value) {
   FileMgr = Value;
-  if (Value)
-VirtualFileSystem = Value->getVirtualFileSystem();
-  else
-VirtualFileSystem.reset();
 }
 
 void CompilerInstance::setSourceManager(SourceManager *Value) {
@@ -297,13 +293,13 @@
 
 // File Manager
 
-FileManager *CompilerInstance::createFileManager() {
-  if (!hasVirtualFileSystem()) {
-IntrusiveRefCntPtr VFS =
-createVFSFromCompilerInvocation(getInvocation(), getDiagnostics());
-setVirtualFileSystem(VFS);
-  }
-  FileMgr = new FileManager(getFileSystemOpts(), VirtualFileSystem);
+FileManager *CompilerInstance::createFileManager(
+IntrusiveRefCntPtr VFS) {
+  if (!VFS && FileMgr)
+VFS = FileMgr->getVirtualFileSystem(); // May be nullptr...
+  if (!VFS)
+VFS = createVFSFromCompilerInvocation(getInvocation(), getDiagnostics());
+  FileMgr = new FileManager(getFileSystemOpts(), std::move(VFS));
   return FileMgr.get();
 }
 
@@ -1101,8 +1097,6 @@
ImportingInstance.getDiagnosticClient()),
  /*ShouldOwnClient=*/true);
 
-  Instance.setVirtualFileSystem(&ImportingInstance.getVirtualFileSystem());
-
   // Note that this module is part of the module build stack, so that we
   // can detect cycles in the module graph.
   Instance.setFileManager(&ImportingInstance.getFileManager());
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1078,28 +1078,29 @@
   if (!Invocation)
 return true;
 
+  if (VFS && FileMgr)
+assert(VFS == FileMgr->getVirtualFileSystem() &&
+   "VFS passed to Parse and VFS in FileMgr are different");
+
   auto CCInvocation = std::make_shared(*Invocation);
   if (OverrideMainBuffer) {
 assert(Preamble &&
"No preamble was built, but OverrideMainBuffer is not null");
-IntrusiveRefCntPtr OldVFS = VFS;
 Pr

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith updated this revision to Diff 190726.
dexonsmith added a comment.

I slightly simplified the code for CompilerInstance::createFileManager 
(replaced with an `assert`), since I noticed that FileManager always constructs 
its own VFS if it isn't passed one.


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

https://reviews.llvm.org/D59377

Files:
  clang-tools-extra/clangd/Compiler.cpp
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -671,7 +671,7 @@
   }
 
   // Make sure clang uses the same VFS as LLDB.
-  instance->setVirtualFileSystem(FileSystem::Instance().GetVirtualFileSystem());
+  instance->createFileManager(FileSystem::Instance().GetVirtualFileSystem());
   instance->setDiagnostics(diagnostics_engine.get());
   instance->setInvocation(invocation);
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -309,8 +309,7 @@
   }
 
   // Make sure clang uses the same VFS as LLDB.
-  m_compiler->setVirtualFileSystem(
-  FileSystem::Instance().GetVirtualFileSystem());
+  m_compiler->createFileManager(FileSystem::Instance().GetVirtualFileSystem());
 
   lldb::LanguageType frame_lang =
   expr.Language(); // defaults to lldb::eLanguageTypeUnknown
Index: clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
+++ clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
@@ -82,8 +82,6 @@
 
   Instance.getDiagnostics().setSourceManager(&SM);
 
-  Instance.setVirtualFileSystem(&CI.getVirtualFileSystem());
-
   // The instance wants to take ownership, however DisableFree frontend option
   // is set to true to avoid double free issues
   Instance.setFileManager(&CI.getFileManager());
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -89,10 +89,6 @@
 
 void CompilerInstance::setFileManager(FileManager *Value) {
   FileMgr = Value;
-  if (Value)
-VirtualFileSystem = Value->getVirtualFileSystem();
-  else
-VirtualFileSystem.reset();
 }
 
 void CompilerInstance::setSourceManager(SourceManager *Value) {
@@ -297,13 +293,14 @@
 
 // File Manager
 
-FileManager *CompilerInstance::createFileManager() {
-  if (!hasVirtualFileSystem()) {
-IntrusiveRefCntPtr VFS =
-createVFSFromCompilerInvocation(getInvocation(), getDiagnostics());
-setVirtualFileSystem(VFS);
-  }
-  FileMgr = new FileManager(getFileSystemOpts(), VirtualFileSystem);
+FileManager *CompilerInstance::createFileManager(
+IntrusiveRefCntPtr VFS) {
+  if (!VFS)
+VFS = FileMgr ? FileMgr->getVirtualFileSystem()
+  : createVFSFromCompilerInvocation(getInvocation(),
+getDiagnostics());
+  assert(VFS && "FileManager has no VFS?");
+  FileMgr = new FileManager(getFileSystemOpts(), std::move(VFS));
   return FileMgr.get();
 }
 
@@ -1101,8 +1098,6 @@
ImportingInstance.getDiagnosticClient()),
  /*ShouldOwnClient=*/true);
 
-  Instance.setVirtualFileSystem(&ImportingInstance.getVirtualFileSystem());
-
   // Note that this module is part of the module build stack, so that we
   // can detect cycles in the module graph.
   Instance.setFileManager(&ImportingInstance.getFileManager());
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1078,28 +1078,29 @@
   if (!Invocation)
 return true;
 
+  if (VFS && FileMgr)
+assert(VFS == FileMgr->getVirtualFileSystem() &&
+   "VFS passed to Parse and VFS in FileMgr are different");
+
   auto CCInvocation = std::make_shared(*Invocation);
   if (OverrideMainBuffer) {
 assert(Preamble &&
"No preamble was built, but OverrideMainBuffer is not null");
-IntrusiveRefCntPtr OldVFS = VFS;
 Preamble->AddImplicitPreamble(*CCInvocation, VFS, OverrideMainBuffer.get());
-if (OldVFS != VFS && FileMgr) {
-  assert(OldVFS == Fi

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith created this revision.
dexonsmith added reviewers: arphaman, Bigcheese, bruno.
Herald added subscribers: jdoerfert, kadircet, jkorous, ioeric.
Herald added a reviewer: martong.
Herald added a reviewer: shafik.

FileManager constructs a VFS in its constructor if it isn't passed one,
and there's no way to reset it.  Make that contract clear by returning a
reference from its accessor.


https://reviews.llvm.org/D59388

Files:
  clang-tools-extra/clang-move/ClangMove.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -44,7 +44,7 @@
   assert(ToAST);
   ASTContext &ToCtx = ToAST->getASTContext();
   auto *OFS = static_cast(
-  ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
+  &ToCtx.getSourceManager().getFileManager().getVirtualFileSystem());
   auto *MFS = static_cast(
   OFS->overlays_begin()->get());
   MFS->addFile(FileName, 0, std::move(Buffer));
Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -301,7 +301,7 @@
   DiagConsumer ? DiagConsumer : &DiagnosticPrinter, false);
 
   const std::unique_ptr Driver(
-  newDriver(&Diagnostics, BinaryName, Files->getVirtualFileSystem()));
+  newDriver(&Diagnostics, BinaryName, &Files->getVirtualFileSystem()));
   // The "input file not found" diagnostics from the driver are useful.
   // The driver is only aware of the VFS working directory, but some clients
   // change this at the FileManager level instead.
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -8378,7 +8378,8 @@
   // We need the native slashes for the actual file system interactions.
   SmallString<128> NativeRelDir = StringRef(RelDir);
   llvm::sys::path::native(NativeRelDir);
-  auto FS = getSourceManager().getFileManager().getVirtualFileSystem();
+  llvm::vfs::FileSystem &FS =
+  getSourceManager().getFileManager().getVirtualFileSystem();
 
   ResultBuilder Results(*this, CodeCompleter->getAllocator(),
 CodeCompleter->getCodeCompletionTUInfo(),
@@ -8424,7 +8425,7 @@
 
 std::error_code EC;
 unsigned Count = 0;
-for (auto It = FS->dir_begin(Dir, EC);
+for (auto It = FS.dir_begin(Dir, EC);
  !EC && It != llvm::vfs::directory_iterator(); It.increment(EC)) {
   if (++Count == 2500) // If we happen to hit a huge directory,
 break; // bail out early so we're not too slow.
Index: clang/lib/Lex/PPLexerChange.cpp
===
--- clang/lib/Lex/PPLexerChange.cpp
+++ clang/lib/Lex/PPLexerChange.cpp
@@ -270,7 +270,7 @@
 
   ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
   const DirectoryEntry *Dir = Mod.getUmbrellaDir().Entry;
-  llvm::vfs::FileSystem &FS = *FileMgr.getVirtualFileSystem();
+  llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
   std::error_code EC;
   for (llvm::vfs::recursive_directory_iterator Entry(FS, Dir->getName(), EC),
End;
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1021,7 +1021,7 @@
 = StringRef(FrameworkDir->getName());
   llvm::sys::path::append(SubframeworksDirName, "Frameworks");
   llvm::sys::path::native(SubframeworksDirName);
-  llvm::vfs::FileSystem &FS = *FileMgr.getVirtualFileSystem();
+  llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
   for (llvm::vfs::directory_iterator
Dir = FS.dir_begin(SubframeworksDirName, EC),
DirEnd;
@@ -2397,7 +2397,7 @@
 std::error_code EC;
 SmallVector Headers;
 llvm::vfs::FileSystem &FS =
-*SourceMgr.getFileManager().getVirtualFileSystem();
+SourceMgr.getFileManager().getVirtualFileSystem();
 for (llvm::vfs::recursive_directory_iterator I(FS, Dir->getName(), EC), E;
  I != E && !EC; I.increment(EC)) {
   if (const FileEntry *FE = SourceMgr.getFileManager().getFile(I->path())) {
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSea

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D59388#1431314 , @jkorous wrote:

> Hi Duncan, thanks for working on better interfaces in clang!
>
> I am just wondering - is it safe to have the lifetime of a single object on 
> heap managed by two different `IntrusiveRefCntPtr` instances?


Yes, it's safe.  The reference count is "intrusive", meaning it's stored in the 
object itself (via inheritance from `RefCountedBase`).  As a result, all the 
instances of `IntrusiveRefCntPtr` that reference at the same object will 
implicitly share their count.


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

https://reviews.llvm.org/D59388



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

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith updated this revision to Diff 191104.
dexonsmith added a comment.

Document the default VFS used by the `FileManager`.


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

https://reviews.llvm.org/D59377

Files:
  clang-tools-extra/clangd/Compiler.cpp
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -671,7 +671,7 @@
   }
 
   // Make sure clang uses the same VFS as LLDB.
-  instance->setVirtualFileSystem(FileSystem::Instance().GetVirtualFileSystem());
+  instance->createFileManager(FileSystem::Instance().GetVirtualFileSystem());
   instance->setDiagnostics(diagnostics_engine.get());
   instance->setInvocation(invocation);
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -309,8 +309,7 @@
   }
 
   // Make sure clang uses the same VFS as LLDB.
-  m_compiler->setVirtualFileSystem(
-  FileSystem::Instance().GetVirtualFileSystem());
+  m_compiler->createFileManager(FileSystem::Instance().GetVirtualFileSystem());
 
   lldb::LanguageType frame_lang =
   expr.Language(); // defaults to lldb::eLanguageTypeUnknown
Index: clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
+++ clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
@@ -82,8 +82,6 @@
 
   Instance.getDiagnostics().setSourceManager(&SM);
 
-  Instance.setVirtualFileSystem(&CI.getVirtualFileSystem());
-
   // The instance wants to take ownership, however DisableFree frontend option
   // is set to true to avoid double free issues
   Instance.setFileManager(&CI.getFileManager());
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -89,10 +89,6 @@
 
 void CompilerInstance::setFileManager(FileManager *Value) {
   FileMgr = Value;
-  if (Value)
-VirtualFileSystem = Value->getVirtualFileSystem();
-  else
-VirtualFileSystem.reset();
 }
 
 void CompilerInstance::setSourceManager(SourceManager *Value) {
@@ -297,13 +293,14 @@
 
 // File Manager
 
-FileManager *CompilerInstance::createFileManager() {
-  if (!hasVirtualFileSystem()) {
-IntrusiveRefCntPtr VFS =
-createVFSFromCompilerInvocation(getInvocation(), getDiagnostics());
-setVirtualFileSystem(VFS);
-  }
-  FileMgr = new FileManager(getFileSystemOpts(), VirtualFileSystem);
+FileManager *CompilerInstance::createFileManager(
+IntrusiveRefCntPtr VFS) {
+  if (!VFS)
+VFS = FileMgr ? FileMgr->getVirtualFileSystem()
+  : createVFSFromCompilerInvocation(getInvocation(),
+getDiagnostics());
+  assert(VFS && "FileManager has no VFS?");
+  FileMgr = new FileManager(getFileSystemOpts(), std::move(VFS));
   return FileMgr.get();
 }
 
@@ -1101,8 +1098,6 @@
ImportingInstance.getDiagnosticClient()),
  /*ShouldOwnClient=*/true);
 
-  Instance.setVirtualFileSystem(&ImportingInstance.getVirtualFileSystem());
-
   // Note that this module is part of the module build stack, so that we
   // can detect cycles in the module graph.
   Instance.setFileManager(&ImportingInstance.getFileManager());
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1078,28 +1078,29 @@
   if (!Invocation)
 return true;
 
+  if (VFS && FileMgr)
+assert(VFS == FileMgr->getVirtualFileSystem() &&
+   "VFS passed to Parse and VFS in FileMgr are different");
+
   auto CCInvocation = std::make_shared(*Invocation);
   if (OverrideMainBuffer) {
 assert(Preamble &&
"No preamble was built, but OverrideMainBuffer is not null");
-IntrusiveRefCntPtr OldVFS = VFS;
 Preamble->AddImplicitPreamble(*CCInvocation, VFS, OverrideMainBuffer.get());
-if (OldVFS != VFS && FileMgr) {
-  assert(OldVFS == FileMgr->getVirtualFileSystem() &&
- "VFS passed to Parse and VFS in FileMgr are diff

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D59377#1431203 , @jkorous wrote:

> In D59377#1430002 , @dexonsmith 
> wrote:
>
> > ... since I noticed that FileManager ...
>
>
> This kind of implies that we should move the comment from `FileManager` 
> constructor implementation to the header thus making it an explicit part of 
> the interface contract.
>
>   // If the caller doesn't provide a virtual file system, just grab the real
>   // file system.
>   
>
> Maybe https://reviews.llvm.org/D59388 would be the right place to do it.


Good call; since I'm relying on it in this patch I've updated this patch to 
document it.


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

https://reviews.llvm.org/D59377



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

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith updated this revision to Diff 191105.
dexonsmith added a comment.

Rebase.


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

https://reviews.llvm.org/D59388

Files:
  clang-tools-extra/clang-move/ClangMove.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -44,7 +44,7 @@
   assert(ToAST);
   ASTContext &ToCtx = ToAST->getASTContext();
   auto *OFS = static_cast(
-  ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
+  &ToCtx.getSourceManager().getFileManager().getVirtualFileSystem());
   auto *MFS = static_cast(
   OFS->overlays_begin()->get());
   MFS->addFile(FileName, 0, std::move(Buffer));
Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -301,7 +301,7 @@
   DiagConsumer ? DiagConsumer : &DiagnosticPrinter, false);
 
   const std::unique_ptr Driver(
-  newDriver(&Diagnostics, BinaryName, Files->getVirtualFileSystem()));
+  newDriver(&Diagnostics, BinaryName, &Files->getVirtualFileSystem()));
   // The "input file not found" diagnostics from the driver are useful.
   // The driver is only aware of the VFS working directory, but some clients
   // change this at the FileManager level instead.
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -8378,7 +8378,8 @@
   // We need the native slashes for the actual file system interactions.
   SmallString<128> NativeRelDir = StringRef(RelDir);
   llvm::sys::path::native(NativeRelDir);
-  auto FS = getSourceManager().getFileManager().getVirtualFileSystem();
+  llvm::vfs::FileSystem &FS =
+  getSourceManager().getFileManager().getVirtualFileSystem();
 
   ResultBuilder Results(*this, CodeCompleter->getAllocator(),
 CodeCompleter->getCodeCompletionTUInfo(),
@@ -8424,7 +8425,7 @@
 
 std::error_code EC;
 unsigned Count = 0;
-for (auto It = FS->dir_begin(Dir, EC);
+for (auto It = FS.dir_begin(Dir, EC);
  !EC && It != llvm::vfs::directory_iterator(); It.increment(EC)) {
   if (++Count == 2500) // If we happen to hit a huge directory,
 break; // bail out early so we're not too slow.
Index: clang/lib/Lex/PPLexerChange.cpp
===
--- clang/lib/Lex/PPLexerChange.cpp
+++ clang/lib/Lex/PPLexerChange.cpp
@@ -270,7 +270,7 @@
 
   ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
   const DirectoryEntry *Dir = Mod.getUmbrellaDir().Entry;
-  llvm::vfs::FileSystem &FS = *FileMgr.getVirtualFileSystem();
+  llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
   std::error_code EC;
   for (llvm::vfs::recursive_directory_iterator Entry(FS, Dir->getName(), EC),
End;
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1021,7 +1021,7 @@
 = StringRef(FrameworkDir->getName());
   llvm::sys::path::append(SubframeworksDirName, "Frameworks");
   llvm::sys::path::native(SubframeworksDirName);
-  llvm::vfs::FileSystem &FS = *FileMgr.getVirtualFileSystem();
+  llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
   for (llvm::vfs::directory_iterator
Dir = FS.dir_begin(SubframeworksDirName, EC),
DirEnd;
@@ -2397,7 +2397,7 @@
 std::error_code EC;
 SmallVector Headers;
 llvm::vfs::FileSystem &FS =
-*SourceMgr.getFileManager().getVirtualFileSystem();
+SourceMgr.getFileManager().getVirtualFileSystem();
 for (llvm::vfs::recursive_directory_iterator I(FS, Dir->getName(), EC), E;
  I != E && !EC; I.increment(EC)) {
   if (const FileEntry *FE = SourceMgr.getFileManager().getFile(I->path())) {
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1579,7 +1579,7 @@
 DirNative);
 
 // Search each of the ".framework" directories to load them as modules.
-llvm::vfs::FileSystem &FS = *FileMgr.getVi

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58548



___
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))

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith edited reviewers, added: erik.pilkington, arphaman; removed: 
dexonsmith.
dexonsmith added a comment.

+Erik and Alex, can you take a look at this?


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] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith added a comment.

Committed r357037.


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

https://reviews.llvm.org/D59377



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

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith closed this revision.
dexonsmith added a comment.

Committed r357038.


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

https://reviews.llvm.org/D59388



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

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D55463#1451166 , @tschuett wrote:

> Are you planning to do this recursively?
>  The minimizer does not help much for the following example, while Sema.h 
> contains 10,000 lines of useless code.
>
>   #include "clang/Sema/Sema.h"
>  
>   int foo() {}
>


Not recursively, but on each file *independently* by intercepting `stat` and 
`open` in a custom VFS layer.  The old patch at https://reviews.llvm.org/D53354 
(which Alex points to above) might give you more context.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55463



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

[PATCH] D60233: [clang-scan-deps] initial outline of the tool that runs preprocessor to find dependencies over a JSON compilation database

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D60233#1467092 , @trixirt wrote:

> A comment by whisperity in the origin WIP did not seem to be addressed.
>  Have you looked at IWYU 
> https://github.com/include-what-you-use/include-what-you-use ?
>  The end goals of clang-scan-deps and iwyu seem similar so their 
> implementation problems would also be similar.
>  The iwyu problems doc is 
> https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYUIsDifficult.md


IWYU doesn't seem related to me.  My understanding is:

- IWYU checks that semantic dependencies between source files is reflected by 
the file dependencies (e.g., semantic dependencies should have matching 
`#include`s).
- `clang-scan-deps` discovers the transitive file dependencies of a TU.

IIUC, IWYU's implementation challenges are related to mapping/discovering 
semantic dependencies between source files, which is something that 
`clang-scan-deps` doesn't care about at all.  Can you clarify what link you see?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60233



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

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D60748#1472829 , @rjmccall wrote:

> I suspect Darwin also doesn't want to take this.  We care very little about 
> 32-bit Intel, and part of caring very little is not wanting to spend any 
> effort dealing with the ramifications of ABI breaks.  That would apply both 
> to the rule in general and to the vector rule specifically.  @dexonsmith, 
> agreed?


Agreed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60748



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D61165#1492724 , @rjmccall wrote:

> In D61165#1492582 , @erik.pilkington 
> wrote:
>
> > Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which 
> > appears to claim that initialization ends with the execution of the 
> > constructor, excluding temporary destructors.
> >
> > > (13) In a non-delegating constructor, initialization proceeds in the 
> > > following order:
> > >  /* list of steps... */
> > >  (13.4) **Finally, the compound-statement of the constructor body is 
> > > executed.**
> >
> > John: do you agree with this analysis?
>
>
> That's talking about constructor bodies.


Sure, but that's all that dcl.int suggests happens during initialization.  
Skipping over paragraphs that don't apply:

> http://eel.is/c++draft/dcl.init#17: The semantics of initializers are as 
> follows.
> 
> - http://eel.is/c++draft/dcl.init#17.6: [I]f the destination type is a 
> (possibly cv-qualified) class type:
>   - http://eel.is/c++draft/dcl.init#17.6.2: [I]f the initialization is 
> direct-initialization, or if it is copy-initialization where the 
> cv-unqualified version of the source type is the same class as, or a derived 
> class of, the class of the destination, constructors are considered. The 
> applicable constructors are enumerated ([over.match.ctor]), and the best one 
> is chosen through overload resolution ([over.match]). **Then:**
> - http://eel.is/c++draft/dcl.init#17.6.2.1: If overload resolution is 
> successful, **the selected constructor is called to initialize the object**, 
> with the initializer expression or expression-list as its argument(s).

Which I read as saying, "the selected constructor is the thing that initializes 
the object".  But, IIUC, you're saying the object isn't necessarily initialized 
when the selected constructor returns.  I can't find any text that would 
support your interpretation.

Let me argue from the other direction as well:

> http://eel.is/c++draft/dcl.init#21: An object whose initialization has 
> completed is deemed to be constructed, [...]

IIUC, you're suggesting that there could be a "constructed, but not yet 
initialized" state, but the definition of "constructed" seems to refute that.


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

https://reviews.llvm.org/D61165



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