[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

So the problem is that there are references to `ParmVarDecl` from inside 
function body and at import of `ParmVarDecl` always a new one is created even 
if there is an already existing (in the existing function prototype)? Maybe it 
works in `VisitParmVarDecl` to search for already existing `ParmVarDecl` (not 
already imported) and return if found.


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

https://reviews.llvm.org/D56936



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

[clang-tools-extra] r351929 - [clangd] Link clangTidy into clangd tests

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Jan 23 00:04:17 2019
New Revision: 351929

URL: http://llvm.org/viewvc/llvm-project?rev=351929&view=rev
Log:
[clangd] Link clangTidy into clangd tests

Patch by Nathan Ridge!

Differential Revision: https://reviews.llvm.org/D57077

Modified:
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

Modified: clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt?rev=351929&r1=351928&r2=351929&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt Wed Jan 23 00:04:17 
2019
@@ -59,6 +59,7 @@ target_link_libraries(ClangdTests
   clangLex
   clangSema
   clangSerialization
+  clangTidy
   clangTooling
   clangToolingCore
   clangToolingInclusions


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

[PATCH] D55400: [analyzer] Move out tracking retain count for OSObjects into a separate checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D55400#1367046 , @george.karpenkov 
wrote:

> > Hmmm, does this mess with options that bad? Could you please clarify?
>
> `registerChecker` gets-or-creates a checker object. A checker name (used for 
> getting the options) is set the first time it's created.
>  The checker which was created first "wins" and gets to name the resulting 
> checker.
>  In practice it basically means that options and checkers reusing the same 
> class do not work.
>  Do you have better ideas on how this could be arranged?


Sure, in fact its already implemented and got accepted by @NoQ, I just didnt 
have the time to commit and fine tune :)

The solution is essentially to create `CheckerManager::getChecker` alongside 
`registerChecker`, and making them in order assert if the checker is 
unregistered, and assert when the checked is already registered. In an earlier 
patch, I implement handling of dependencies on a higher level, essentially 
making `CheckerRegistry` take care of this, instead of making checker registry 
functions register multiple checkers, or, like in this case, register a common 
base. Before the registry function for either RetainCound or 
OSObjectRetainCount is called (which will now invoke `getChecker`), the 
registry function for RetainCountBase will be called, that will register the 
actual checker object.

You can take a look at how this would look like in D54438 
.

Would love to hear your thoughts on this! :)

> I think the current situation is a mess - ideally I would prefer to be able 
> to access the options in the constructor, but we can't even do that,
>  since `registerChecker` sets the checker name and is called after the object 
> has been constructed.
>  It seems that it would only make sense if the checker name is known at the 
> time the checker is constructed: probably the function `registerXChecker` 
> should get it as an argument.

The proposed solution solves this issue as well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55400



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

[PATCH] D57062: [analyzer] Re-enable the "System is over constrained" assertion on optimized builds.

[Kristóf Umann via Phabricator via cfe-commits]

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

Hmmm, came across this one in the not too distant future, and always wondered 
how painful that performance hit would be that even Release+Asserts should be 
spared from it. I think 5% performance hit, if asserts are enabled, is an 
acceptable tradeoff, if the assert is crucial.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57062



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

[PATCH] D57062: [analyzer] Re-enable the "System is over constrained" assertion on optimized builds.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

And if we find out that this is far too painful, we can always put it back 
anyways. Doubt that'll happen though.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57062



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

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: alexfh, JonasToth, hokein, Eugene.Zelenko, 
aaron.ballman.
MyDeveloperDay added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.

The usefulness of **modernize-use-override** can be reduced if you have to live 
in an environment where you support multiple compilers, some of which sadly are 
not yet fully C++11 compliant

some codebases have to use override as a macro OVERRIDE e.g.

  #if defined(COMPILER_MSVC)
  #define OVERRIDE override
  #elif defined(__clang__)
  #define OVERRIDE override
  // GCC 4.7 supports explicit virtual overrides when C++11 support is enabled.
  #define OVERRIDE override
  #else
  #define OVERRIDE
  #endif

This allows code to be compiled with C++11 compliant compilers and get warnings 
and errors that clang, MSVC,gcc can give, while still allowing other legacy pre 
C++11 compilers to compile the code. This can be an important step towards 
modernizing C++ code whilst living in a legacy codebase.

When it comes to clang tidy, the use of the **modernize-use-override** is one 
of the most useful checks, but the messages reported are inaccurate for that 
codebase if the standard approach is to use the macros OVERRIDE and/or FINAL.

When combined with fix-its that introduce the C++11 override keyword, they 
become fatal, resulting in the modernize-use-override check being turned off to 
prevent the introduction of such errors.

This revision, allows the possibility for the replacement **override **to be a 
macro instead, Allowing the clang-tidy check to be run on  both pre and post 
C++11 code, and allowing fix-its to be applied.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D57087

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tidy/modernize/UseOverrideCheck.h
  docs/clang-tidy/checks/modernize-use-override.rst
  test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
  test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp

Index: test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'CUSTOM_OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+// As if the macro was not defined.
+//#define CUSTOM_OVERRIDE override
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-FIXES: {{^}}  virtual ~SimpleCases();
+
+  void a();
+  // CHECK-FIXES: {{^}}  void a();
+
+  virtual void b();
+  // CHECK-FIXES: {{^}}  virtual void b();
+};
Index: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
@@ -0,0 +1,95 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define UNUSED __attribute__((unused))
+
+struct MUST_USE_RESULT MustUseResultObject {};
+
+struct IntPair {
+  int First, Second;
+};
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void d();
+  virtual void d2();
+  virtual void e() = 0;
+  virtual void f() = 0;
+  virtual void f2() const = 0;
+  virtual void g() = 0;
+
+  virtual void j() const;
+  virtual MustUseResultObject k();
+  virtual bool l() MUST_USE_RESULT UNUSED;
+  virtual bool n() MUST_USE_RESULT UNUSED;
+
+  virtual void m();
+  virtual void m2();
+  virtual void o() __attribute__((unused));
+
+  virtual void r() &;
+  virtual void rr() &&;
+
+  virtual void cv() const volatile;
+  virtual void cv2() const volatile;
+
+  virtual void ne() noexcept(false);
+  virtual void t() throw();
+
+  virtual void il(IntPair);
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~SimpleCases() OVERRIDE;
+
+  void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'OVERRIDE' or (rarely) 'FINAL' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() OVERRIDE;
+
+  virtual void b();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefe

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 183063.
MyDeveloperDay added a comment.

Adding release note change


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

https://reviews.llvm.org/D57087

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tidy/modernize/UseOverrideCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-override.rst
  test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
  test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp

Index: test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'CUSTOM_OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+// As if the macro was not defined.
+//#define CUSTOM_OVERRIDE override
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-FIXES: {{^}}  virtual ~SimpleCases();
+
+  void a();
+  // CHECK-FIXES: {{^}}  void a();
+
+  virtual void b();
+  // CHECK-FIXES: {{^}}  virtual void b();
+};
Index: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
@@ -0,0 +1,95 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define UNUSED __attribute__((unused))
+
+struct MUST_USE_RESULT MustUseResultObject {};
+
+struct IntPair {
+  int First, Second;
+};
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void d();
+  virtual void d2();
+  virtual void e() = 0;
+  virtual void f() = 0;
+  virtual void f2() const = 0;
+  virtual void g() = 0;
+
+  virtual void j() const;
+  virtual MustUseResultObject k();
+  virtual bool l() MUST_USE_RESULT UNUSED;
+  virtual bool n() MUST_USE_RESULT UNUSED;
+
+  virtual void m();
+  virtual void m2();
+  virtual void o() __attribute__((unused));
+
+  virtual void r() &;
+  virtual void rr() &&;
+
+  virtual void cv() const volatile;
+  virtual void cv2() const volatile;
+
+  virtual void ne() noexcept(false);
+  virtual void t() throw();
+
+  virtual void il(IntPair);
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~SimpleCases() OVERRIDE;
+
+  void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'OVERRIDE' or (rarely) 'FINAL' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() OVERRIDE;
+
+  virtual void b();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void b() OVERRIDE;
+
+  virtual void c();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void c() OVERRIDE;
+
+  virtual void e() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void e() OVERRIDE = 0;
+
+  virtual void f()=0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f() OVERRIDE =0;
+
+  virtual void f2() const=0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f2() const OVERRIDE =0;
+
+  virtual void g() ABSTRACT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void g() OVERRIDE ABSTRACT;
+
+  virtual void j() const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void j() const OVERRIDE;
+
+  virtual MustUseResultObject k();  // Has an implicit attribute.
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: prefer using
+  // CHECK-FIXES: {{^}}  MustUseResultObject k() OVERRIDE;
+};
Index: docs/clang-tidy/checks/modernize-use-override.rst
===
--- docs/clang-tidy/checks/modernize-use-override.rst
+++ docs/clang-tidy/checks/modernize-use-override.rst
@@ -3,5 +3,37 @@
 modernize-use-override
 ==
 
+Adds ``override`` (introduced in C++11) to overridden 

[PATCH] D56925: Do not use frame pointer by default for MSP430

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Thanks for adding a test case! I know not enough about msp430/clang to verify 
it, unfortunately.
(I just asked for a test because it is always useful to have one for any 
non-NFC patch).


Repository:
  rC Clang

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

https://reviews.llvm.org/D56925



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

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> So the problem is that there are references to ParmVarDecl from inside 
> function body and at import of ParmVarDecl always a new one is created even 
> if there is an already existing (in the existing function prototype)?

Yes. During the import of the body we import a `DeclRefExpr` which refers to a 
`ParmVarDecl` but not the existing one, thus a new is created.

> Maybe it works in VisitParmVarDecl to search for already existing ParmVarDecl 
> (not already imported) and return if found.

Yes it would worth to try, indeed.


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

https://reviews.llvm.org/D56936



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

[PATCH] D56924: Handle ObjCCategoryDecl class extensions for print

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This is definitely an improvement, though I don't know if it's *right*. 
@akyrtzi, thoughts?




Comment at: unittests/AST/NamedDeclPrinterTest.cpp:206
+"property",
+"(class extension)::property"));
+}

One concern is that similarly-named members in extensions of different classes 
will have the same name (whereas categories have names, so the collision is 
less likely).
It's also harder to go from printed name -> source code.

`Obj::(class extension)::property` **might** be a more useful QName (including 
*also* the class name).
But I don't know obj-c well, would be good if Apple folks could chime in.

`(class extension)::property` is definitely an improvement over the current 
`::property`, though.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56924



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

Re: [clang-tools-extra] r351788 - [clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB.

[Kadir Çetinkaya via cfe-commits]

Hey Hans,

There is actually a parent for this patch at
https://reviews.llvm.org/rC351531 and it hasn't been merged.

In addition to that not exactly as a follow-up, but there is
https://reviews.llvm.org/rCTE351738 which is kind of related to this two
patches, you might also consider merging that but it is more of a NFC.

On Tue, Jan 22, 2019 at 8:08 PM Hans Wennborg  wrote:

> This has been merged to the 8.0 branch in r351860. Please  let me know
> if there are any follow-ups so they can be merged too.
>
> Thanks,
> Hans
>
> On Tue, Jan 22, 2019 at 1:10 AM Kadir Cetinkaya via cfe-commits
>  wrote:
> >
> > Author: kadircet
> > Date: Tue Jan 22 01:10:20 2019
> > New Revision: 351788
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=351788&view=rev
> > Log:
> > [clangd] Filter out plugin related flags and move all commandline
> manipulations into OverlayCDB.
> >
> > Summary:
> > Some projects make use of clang plugins when building, but clangd is
> > not aware of those plugins therefore can't work with the same compile
> command
> > arguments.
> >
> > There were multiple places clangd performed commandline manipulations,
> >  this one also moves them all into OverlayCDB.
> >
> > Reviewers: ilya-biryukov
> >
> > Subscribers: klimek, sammccall, ioeric, MaskRay, jkorous, arphaman,
> cfe-commits
> >
> > Differential Revision: https://reviews.llvm.org/D56841
> >
> > Modified:
> > clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
> > clang-tools-extra/trunk/clangd/ClangdServer.cpp
> > clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
> > clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
> > clang-tools-extra/trunk/clangd/index/Background.cpp
> > clang-tools-extra/trunk/clangd/index/Background.h
> > clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
> > clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
> >
>  clang-tools-extra/trunk/unittests/clangd/GlobalCompilationDatabaseTests.cpp
> >
> > Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=351788&r1=351787&r2=351788&view=diff
> >
> ==
> > --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
> > +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Tue Jan 22
> 01:10:20 2019
> > @@ -289,7 +289,8 @@ void ClangdLSPServer::onInitialize(const
> >if (UseDirBasedCDB)
> >  BaseCDB =
> llvm::make_unique(
> >  CompileCommandsDir);
> > -  CDB.emplace(BaseCDB.get(),
> Params.initializationOptions.fallbackFlags);
> > +  CDB.emplace(BaseCDB.get(), Params.initializationOptions.fallbackFlags,
> > +  ClangdServerOpts.ResourceDir);
> >Server.emplace(*CDB, FSProvider, static_cast &>(*this),
> >   ClangdServerOpts);
> >applyConfiguration(Params.initializationOptions.ConfigSettings);
> >
> > Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=351788&r1=351787&r2=351788&view=diff
> >
> ==
> > --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
> > +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Tue Jan 22 01:10:20
> 2019
> > @@ -37,11 +37,6 @@ namespace clang {
> >  namespace clangd {
> >  namespace {
> >
> > -std::string getStandardResourceDir() {
> > -  static int Dummy; // Just an address in this process.
> > -  return CompilerInvocation::GetResourcesPath("clangd", (void *)&Dummy);
> > -}
> > -
> >  class RefactoringResultCollector final
> >  : public tooling::RefactoringResultConsumer {
> >  public:
> > @@ -107,8 +102,6 @@ ClangdServer::ClangdServer(const GlobalC
> > DiagnosticsConsumer &DiagConsumer,
> > const Options &Opts)
> >  : CDB(CDB), FSProvider(FSProvider),
> > -  ResourceDir(Opts.ResourceDir ? *Opts.ResourceDir
> > -   : getStandardResourceDir()),
> >DynamicIdx(Opts.BuildDynamicSymbolIndex
> >   ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
> >   : nullptr),
> > @@ -136,7 +129,7 @@ ClangdServer::ClangdServer(const GlobalC
> >  AddIndex(Opts.StaticIndex);
> >if (Opts.BackgroundIndex) {
> >  BackgroundIdx = llvm::make_unique(
> > -Context::current().clone(), ResourceDir, FSProvider, CDB,
> > +Context::current().clone(), FSProvider, CDB,
> >  BackgroundIndexStorage::createDiskBackedStorageFactory(),
> >  Opts.BackgroundIndexRebuildPeriodMs);
> >  AddIndex(BackgroundIdx.get());
> > @@ -461,10 +454,6 @@ tooling::CompileCommand ClangdServer::ge
> >llvm::Optional C =
> CDB.getCompileCommand(File);
> >if (!C) // FIXME: Suppress diagnostics? Let the user k

[clang-tools-extra] r351941 - [clangd] Fix crash due to ObjCPropertyDecl

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Wed Jan 23 02:35:12 2019
New Revision: 351941

URL: http://llvm.org/viewvc/llvm-project?rev=351941&view=rev
Log:
[clangd] Fix crash due to ObjCPropertyDecl

With ObjCPropertyDecl, ASTNode.OrigD can be a ObjCPropertyImplDecl
which is not a NamedDecl, leading to a crash since the code
incorrectly assumes ASTNode.OrigD will always be a NamedDecl.

Change by dgoldman (David Goldman)!

Differential Revision: https://reviews.llvm.org/D56916

Modified:
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=351941&r1=351940&r2=351941&view=diff
==
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Wed Jan 23 
02:35:12 2019
@@ -347,19 +347,25 @@ bool SymbolCollector::handleDeclOccurenc
   if (!ID)
 return true;
 
-  const NamedDecl &OriginalDecl = *cast(ASTNode.OrigD);
+  // FIXME: ObjCPropertyDecl are not properly indexed here:
+  // - ObjCPropertyDecl may have an OrigD of ObjCPropertyImplDecl, which is
+  // not a NamedDecl.
+  auto *OriginalDecl = dyn_cast(ASTNode.OrigD);
+  if (!OriginalDecl)
+return true;
+
   const Symbol *BasicSymbol = Symbols.find(*ID);
   if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
 BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
-  else if (isPreferredDeclaration(OriginalDecl, Roles))
+  else if (isPreferredDeclaration(*OriginalDecl, Roles))
 // If OriginalDecl is preferred, replace the existing canonical
 // declaration (e.g. a class forward declaration). There should be at most
 // one duplicate as we expect to see only one preferred declaration per
 // TU, because in practice they are definitions.
-BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID), IsMainFileOnly);
+BasicSymbol = addDeclaration(*OriginalDecl, std::move(*ID), 
IsMainFileOnly);
 
   if (Roles & static_cast(index::SymbolRole::Definition))
-addDefinition(OriginalDecl, *BasicSymbol);
+addDefinition(*OriginalDecl, *BasicSymbol);
   return true;
 }
 

Modified: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp?rev=351941&r1=351940&r2=351941&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp Wed Jan 
23 02:35:12 2019
@@ -437,6 +437,21 @@ TEST_F(SymbolCollectorTest, ObjCSymbols)
   QName("MyProtocol"), QName("MyProtocol::someMethodName3:")));
 }
 
+TEST_F(SymbolCollectorTest, ObjCPropertyImpl) {
+  const std::string Header = R"(
+@interface Container
+@property(nonatomic) int magic;
+@end
+
+@implementation Container
+@end
+  )";
+  TestFileName = testPath("test.m");
+  runSymbolCollector(Header, /*Main=*/"", {"-xobjective-c++"});
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Container"),
+QName("Container::magic")));
+}
+
 TEST_F(SymbolCollectorTest, Locations) {
   Annotations Header(R"cpp(
 // Declared in header, defined in main.


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

[PATCH] D56916: Fix crash due to ObjCPropertyDecl

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL351941: [clangd] Fix crash due to ObjCPropertyDecl (authored 
by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56916?vs=182942&id=183068#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56916

Files:
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp


Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -437,6 +437,21 @@
   QName("MyProtocol"), QName("MyProtocol::someMethodName3:")));
 }
 
+TEST_F(SymbolCollectorTest, ObjCPropertyImpl) {
+  const std::string Header = R"(
+@interface Container
+@property(nonatomic) int magic;
+@end
+
+@implementation Container
+@end
+  )";
+  TestFileName = testPath("test.m");
+  runSymbolCollector(Header, /*Main=*/"", {"-xobjective-c++"});
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Container"),
+QName("Container::magic")));
+}
+
 TEST_F(SymbolCollectorTest, Locations) {
   Annotations Header(R"cpp(
 // Declared in header, defined in main.
Index: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
@@ -347,19 +347,25 @@
   if (!ID)
 return true;
 
-  const NamedDecl &OriginalDecl = *cast(ASTNode.OrigD);
+  // FIXME: ObjCPropertyDecl are not properly indexed here:
+  // - ObjCPropertyDecl may have an OrigD of ObjCPropertyImplDecl, which is
+  // not a NamedDecl.
+  auto *OriginalDecl = dyn_cast(ASTNode.OrigD);
+  if (!OriginalDecl)
+return true;
+
   const Symbol *BasicSymbol = Symbols.find(*ID);
   if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
 BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
-  else if (isPreferredDeclaration(OriginalDecl, Roles))
+  else if (isPreferredDeclaration(*OriginalDecl, Roles))
 // If OriginalDecl is preferred, replace the existing canonical
 // declaration (e.g. a class forward declaration). There should be at most
 // one duplicate as we expect to see only one preferred declaration per
 // TU, because in practice they are definitions.
-BasicSymbol = addDeclaration(OriginalDecl, std::move(*ID), IsMainFileOnly);
+BasicSymbol = addDeclaration(*OriginalDecl, std::move(*ID), 
IsMainFileOnly);
 
   if (Roles & static_cast(index::SymbolRole::Definition))
-addDefinition(OriginalDecl, *BasicSymbol);
+addDefinition(*OriginalDecl, *BasicSymbol);
   return true;
 }
 


Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -437,6 +437,21 @@
   QName("MyProtocol"), QName("MyProtocol::someMethodName3:")));
 }
 
+TEST_F(SymbolCollectorTest, ObjCPropertyImpl) {
+  const std::string Header = R"(
+@interface Container
+@property(nonatomic) int magic;
+@end
+
+@implementation Container
+@end
+  )";
+  TestFileName = testPath("test.m");
+  runSymbolCollector(Header, /*Main=*/"", {"-xobjective-c++"});
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Container"),
+QName("Container::magic")));
+}
+
 TEST_F(SymbolCollectorTest, Locations) {
   Annotations Header(R"cpp(
 // Declared in header, defined in main.
Index: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
@@ -347,19 +347,25 @@
   if (!ID)
 return true;
 
-  const NamedDecl &OriginalDecl = *cast(ASTNode.OrigD);
+  // FIXME: ObjCPropertyDecl are not properly indexed here:
+  // - ObjCPropertyDecl may have an OrigD of ObjCPropertyImplDecl, which is
+  // not a NamedDecl.
+  auto *OriginalDecl = dyn_cast(ASTNode.OrigD);
+  if (!OriginalDecl)
+return true;
+
   const Symbol *BasicSymbol = Symbols.find(*ID);
   if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
 BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
-  else if (isPreferredDeclaration(OriginalDecl, Roles))
+  else if (isPreferredDeclaration(*OriginalDecl, Roles))
 // If OriginalDecl is pr

[PATCH] D56916: Fix crash due to ObjCPropertyDecl

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Done. I had to remove `Container::_magic` from the list of expeced symbols to 
make the test pass, let me know if it should actually be there.
Again, thanks for the fix!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56916



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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Sam McCall via cfe-commits]

Ugh, sorry about this :-\

Some random observations:
 - I didn't think clang itself could actually hit that codepath (open=false
then open=true for the same file). Though not surprising it's
precompiled-headers related, that's similar to how clangd hit it.
 - The obvious failure mode (file state actually changed on disk) isn't the
case. It's possible that the VFS or filemanager state might be different at
the two relevant points in time.
 - this was a bugfix itself, and it seems plausible that we're uncovering
another subtle latent bug. So I'd really like to get a bit more info before
reverting.
 - that said, this seems like an issue that needs to be fixed on the llvm8
branch somehow.

If it's easy enough to do so, it'd be useful to add logging to
FileManager::getStatValue...
if Path ends in "content_security_policy_parsers.h" then log:
 - Path (the full path we're opening)
 - FS->getCurrentWorkingDirectory() (in case Path is relative)
 - whether the file pointer F is null (if non-null, this is an open)
 - and the return value of FileSystemStatCache::get() (was the file found
or not)
I expect to see a call with F != null followed by a call with F == null.

I can also try to repro locally, though I don't have a mac or experience
building chromium, and it sounds likely this is at least somewhat
platform-dependent.

On Wed, Jan 23, 2019 at 4:17 AM Nico Weber via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> I don't have a reduced test case yet, but this seems to cause clang to
> sometimes claim that an included file isn't found even if it's there, at
> least on macOS:
> https://bugs.chromium.org/p/chromium/issues/detail?id=924225
>
> On Mon, Nov 19, 2018 at 8:40 AM Sam McCall via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: sammccall
>> Date: Mon Nov 19 05:37:46 2018
>> New Revision: 347205
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=347205&view=rev
>> Log:
>> [FileManager] getFile(open=true) after getFile(open=false) should open
>> the file.
>>
>> Summary:
>> Old behavior is to just return the cached entry regardless of opened-ness.
>> That feels buggy (though I guess nobody ever actually needed this).
>>
>> This came up in the context of clangd+clang-tidy integration: we're
>> going to getFile(open=false) to replay preprocessor actions obscured by
>> the preamble, but the compilation may subsequently getFile(open=true)
>> for non-preamble includes.
>>
>> Reviewers: ilya-biryukov
>>
>> Subscribers: ioeric, kadircet, cfe-commits
>>
>> Differential Revision: https://reviews.llvm.org/D54691
>>
>> Modified:
>> cfe/trunk/include/clang/Basic/FileManager.h
>> cfe/trunk/lib/Basic/FileManager.cpp
>> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/FileManager.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=347205&r1=347204&r2=347205&view=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Basic/FileManager.h (original)
>> +++ cfe/trunk/include/clang/Basic/FileManager.h Mon Nov 19 05:37:46 2018
>> @@ -70,14 +70,15 @@ class FileEntry {
>>bool IsNamedPipe;
>>bool InPCH;
>>bool IsValid;   // Is this \c FileEntry initialized and
>> valid?
>> +  bool DeferredOpen;  // Created by getFile(OpenFile=0); may
>> open later.
>>
>>/// The open file, if it is owned by the \p FileEntry.
>>mutable std::unique_ptr File;
>>
>>  public:
>>FileEntry()
>> -  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
>> -  {}
>> +  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
>> +DeferredOpen(false) {}
>>
>>FileEntry(const FileEntry &) = delete;
>>FileEntry &operator=(const FileEntry &) = delete;
>>
>> Modified: cfe/trunk/lib/Basic/FileManager.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=347205&r1=347204&r2=347205&view=diff
>>
>> ==
>> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
>> +++ cfe/trunk/lib/Basic/FileManager.cpp Mon Nov 19 05:37:46 2018
>> @@ -221,15 +221,21 @@ const FileEntry *FileManager::getFile(St
>>*SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
>>
>>// See if there is already an entry in the map.
>> -  if (NamedFileEnt.second)
>> -return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
>> -:
>> NamedFileEnt.second;
>> +  if (NamedFileEnt.second) {
>> +if (NamedFileEnt.second == NON_EXISTENT_FILE)
>> +  return nullptr;
>> +// Entry exists: return it *unless* it wasn't opened and open is
>> requested.
>> +if (!(NamedFileEnt.second->DeferredOpen && openFile))
>> +  return NamedFileEnt.second;
>> +// We previously stat()ed the file, but didn't open it: do that
>> below.
>> +// FIXME: the be

[clang-tools-extra] r351943 - [clangd] Workaround a test failure after r351941

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Wed Jan 23 03:32:07 2019
New Revision: 351943

URL: http://llvm.org/viewvc/llvm-project?rev=351943&view=rev
Log:
[clangd] Workaround a test failure after r351941

This should fix failing buildbots.

Modified:
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Modified: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp?rev=351943&r1=351942&r2=351943&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp Wed Jan 
23 03:32:07 2019
@@ -448,8 +448,10 @@ TEST_F(SymbolCollectorTest, ObjCProperty
   )";
   TestFileName = testPath("test.m");
   runSymbolCollector(Header, /*Main=*/"", {"-xobjective-c++"});
-  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Container"),
-QName("Container::magic")));
+  EXPECT_THAT(Symbols, Contains(QName("Container")));
+  EXPECT_THAT(Symbols, Contains(QName("Container::magic")));
+  // FIXME: Results also contain Container::_magic on some platforms.
+  //Figure out why it's platform-dependent.
 }
 
 TEST_F(SymbolCollectorTest, Locations) {


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

[PATCH] D56935: [NewPM] Add support for new-PM plugins to clang

[Philip Pfaffe via Phabricator via cfe-commits]

philip.pfaffe added a comment.

I'm not sure what the current state of plugins on windows is. They were broken 
and disabled last time I worked on this, but that might've changed in the 
meantime! Worth checking.




Comment at: clang/include/clang/Basic/CodeGenOptions.h:292
+  /// List of dynamic shared object files to be loaded as pass plugins.
+  std::vector PassPlugins;
+

melver wrote:
> philip.pfaffe wrote:
> > This  should be SmallVector.
> Not sure if this is better. getAllArgValues returns a vector, which 
> is why I think the above members are also vector. And std::vector 
> cannot be assigned to SmallVector, which required an extra line in 
> CompilerInvocation.cpp.
> 
> Let me know what you think.
You're right, SmallVector doesn't actually help here. Sorry!


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

https://reviews.llvm.org/D56935



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

[PATCH] D56916: Fix crash due to ObjCPropertyDecl

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

It looks like `Container::_magic` is a platform-dependent completion, I don't 
have it on Linux, but 
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/42665
 fails because it's not in the list.
Submitted rL351943  to workaround the 
failure. @dgoldman, any idea why completion might be there on some platforms, 
but not the others?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56916



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

[PATCH] D57093: [CodeComplete] [clangd] Fix crash on ValueDecl with a null type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added a reviewer: kadircet.
Herald added subscribers: arphaman, jkorous, MaskRay, ioeric.

https://reviews.llvm.org/D57093

Files:
  clang-tools-extra/clangd/ExpectedTypes.cpp
  clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/crash-null-type.cpp


Index: clang/test/CodeCompletion/crash-null-type.cpp
===
--- /dev/null
+++ clang/test/CodeCompletion/crash-null-type.cpp
@@ -0,0 +1,8 @@
+void test() {
+  for (auto [loopVar] : y) { // y has to be unresolved
+loopVa
+  }
+}
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:3:11 %s -o - \
+// RUN:| FileCheck %s
+// CHECK: COMPLETION: loopVar
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -680,7 +680,8 @@
 T = Property->getType();
   else if (const auto *Value = dyn_cast(ND))
 T = Value->getType();
-  else
+
+  if (T.isNull())
 return QualType();
 
   // Dig through references, function pointers, and block pointers to
Index: clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
@@ -2319,6 +2319,17 @@
   EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
 }
 
+TEST(CompletionTest, CrashOnNullType) {
+  auto Results = completions(R"cpp(
+int main() {
+  for (auto [loopVar] : y ) { // y has to be unresolved.
+int z = loopV^;
+  }
+}
+  )cpp").Completions;
+  EXPECT_THAT(Results, ElementsAre(Named("loopVar")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ExpectedTypes.cpp
===
--- clang-tools-extra/clangd/ExpectedTypes.cpp
+++ clang-tools-extra/clangd/ExpectedTypes.cpp
@@ -35,8 +35,10 @@
 typeOfCompletion(const CodeCompletionResult &R) {
   auto *VD = dyn_cast_or_null(R.Declaration);
   if (!VD)
-return None; // We handle only variables and functions below.
+return llvm::None; // We handle only variables and functions below.
   auto T = VD->getType();
+  if (T.isNull())
+return llvm::None;
   if (auto FuncT = T->getAs()) {
 // Functions are a special case. They are completed as 'foo()' and we want
 // to match their return type rather than the function type itself.


Index: clang/test/CodeCompletion/crash-null-type.cpp
===
--- /dev/null
+++ clang/test/CodeCompletion/crash-null-type.cpp
@@ -0,0 +1,8 @@
+void test() {
+  for (auto [loopVar] : y) { // y has to be unresolved
+loopVa
+  }
+}
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:3:11 %s -o - \
+// RUN:| FileCheck %s
+// CHECK: COMPLETION: loopVar
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -680,7 +680,8 @@
 T = Property->getType();
   else if (const auto *Value = dyn_cast(ND))
 T = Value->getType();
-  else
+
+  if (T.isNull())
 return QualType();
 
   // Dig through references, function pointers, and block pointers to
Index: clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
@@ -2319,6 +2319,17 @@
   EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
 }
 
+TEST(CompletionTest, CrashOnNullType) {
+  auto Results = completions(R"cpp(
+int main() {
+  for (auto [loopVar] : y ) { // y has to be unresolved.
+int z = loopV^;
+  }
+}
+  )cpp").Completions;
+  EXPECT_THAT(Results, ElementsAre(Named("loopVar")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ExpectedTypes.cpp
===
--- clang-tools-extra/clangd/ExpectedTypes.cpp
+++ clang-tools-extra/clangd/ExpectedTypes.cpp
@@ -35,8 +35,10 @@
 typeOfCompletion(const CodeCompletionResult &R) {
   auto *VD = dyn_cast_or_null(R.Declaration);
   if (!VD)
-return None; // We handle only variables and functions below.
+return llvm::None; // We handle only variables and functions below.
   auto T = VD->getType();
+  if (T.isNull())
+return llvm::None;
   if (auto FuncT = T->getAs()) {
 // Functions are a special case. They are completed as 'foo()' and we want
 // to match their return type rather than the function type itself.
___

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[Alexander Kornienko via Phabricator via cfe-commits]

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

I tend to think that a better migration strategy is to change the compiler to a 
C++11-compatible one first, and then turn on C++11 mode and migrate the code 
(possibly file-by-file or with a different granularity). But if you observe a 
situation where compatibility macros for C++11 constructs are actually a better 
way to migrate, then the proposed functionality makes sense.




Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:32
+: ClangTidyCheck(Name, Context),
+  OverrideMacro(Options.get("OverrideMacro", "override")),
+  FinalMacro(Options.get("FinalMacro", "final")) {}

I'd suggest to default to an empty string and use `override` as a fallback 
right in the code where the diagnostic is generated.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:42-46
+  // If we use ``override``, we require at least C++11. Use a
+  // macro with pre c++11 compilers by using OverrideMacro option.
+  if ((OverrideMacro == "override" && !getLangOpts().CPlusPlus11) ||
+  !getLangOpts().CPlusPlus)
+return;

I think, this should be left as is, because
1. clang-tidy understands C++11 and it makes sense to run it in C++11 mode to 
get more information out of compiler (e.g. then OVERRIDE macro will actually be 
defined to `override` and clang-tidy wouldn't have to jump through the hoops to 
detect that a method is already declared `override`).
2. I've seen folks who just `#define override` in pre-C++11 mode to make the 
code compile.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:97
+
+  if (!doesOverrideMacroExist(Context, OverrideMacro))
+return;

The utility of the function is questionable. I'd drop it and replace the call 
with `if (!OverrideMacro.empty() && 
!Context.Idents.get(OverrideMacro).hasMacroDefinition()) ...`.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:121-131
+Message = "prefer using '" + OverrideMacro + "' or (rarely) '" +
+  FinalMacro + "' instead of 'virtual'";
   } else if (KeywordCount == 0) {
-Message = "annotate this function with 'override' or (rarely) 'final'";
+Message = "annotate this function with '" + OverrideMacro +
+  "' or (rarely) '" + FinalMacro + "'";
   } else {
 StringRef Redundant =

How about using diagnostic arguments instead of string concatenation 
(`diag(Loc, "prefer using %1 or %2") << Macro1 << Macro2;`)?



Comment at: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp:95
+  // CHECK-FIXES: {{^}}  MustUseResultObject k() OVERRIDE;
+};

Please add a test where the OVERRIDE macro is already present. Same for the 
FINAL macro.


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

https://reviews.llvm.org/D57087



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

[PATCH] D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:517
+  has(compoundStmt(
+  
has(caseStmt(hasDescendant(ifStmt(hasThen(returnsBool(Value)),
+unless(hasElse(stmt(

Please put the more expensive matcher (`hasDescendant` may be *really* 
expensive) last. Same below.


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

https://reviews.llvm.org/D56303



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

[PATCH] D56849: [ASTMatchers][NFC] Update comments on assorted `CXXMemberCallExpr` matchers.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.

LG


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

https://reviews.llvm.org/D56849



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

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:136
+  case Stmt::UnaryExprOrTypeTraitExprClass:
+if (cast(Left)->isArgumentType() &&
+cast(Right)->isArgumentType())

Any reasons not to pull out the results of `cast<>()` to local variables? In 
this particular case this would make the code look much simpler.



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:613
 
+static bool isSameToken(Token &T1, Token &T2, const SourceManager &SM) {
+  if (T1.getKind()!=T2.getKind())

`const Token&`



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:614
+static bool isSameToken(Token &T1, Token &T2, const SourceManager &SM) {
+  if (T1.getKind()!=T2.getKind())
+return false;

clang-format, please



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:616
+return false;
+  else if (T1.isNot(tok::raw_identifier))
+return true;

https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:668-669
+   isSameToken(LTok, RTok, SM) &&
+   getCharCountUntilEndOfExpr(Lsr, LTok, SM) > 0 &&
+   getCharCountUntilEndOfExpr(Rsr, RTok, SM) > 0);
+  return !((getCharCountUntilEndOfExpr(Lsr, LTok, SM) == 0 &&

Can you just compare the location of (the end of?) the token with the end of 
the range?


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

https://reviews.llvm.org/D55125



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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

On Wed, Jan 23, 2019 at 5:44 AM Sam McCall  wrote:

> Ugh, sorry about this :-\
>
> Some random observations:
>  - I didn't think clang itself could actually hit that codepath
> (open=false then open=true for the same file). Though not surprising it's
> precompiled-headers related, that's similar to how clangd hit it.
>  - The obvious failure mode (file state actually changed on disk) isn't
> the case. It's possible that the VFS or filemanager state might be
> different at the two relevant points in time.
>  - this was a bugfix itself, and it seems plausible that we're uncovering
> another subtle latent bug. So I'd really like to get a bit more info before
> reverting.
>  - that said, this seems like an issue that needs to be fixed on the llvm8
> branch somehow.
>
> If it's easy enough to do so, it'd be useful to add logging to
> FileManager::getStatValue...
> if Path ends in "content_security_policy_parsers.h" then log:
>  - Path (the full path we're opening)
>  - FS->getCurrentWorkingDirectory() (in case Path is relative)
>  - whether the file pointer F is null (if non-null, this is an open)
>  - and the return value of FileSystemStatCache::get() (was the file found
> or not)
> I expect to see a call with F != null followed by a call with F == null.
>

I can repro locally, happy to try everything you want me to try. I had
reduced the repro a bit so now the error looks like:

../../third_party/blink/renderer/platform/scheduler/public/frame_or_worker_scheduler.h(10,10):
fatal error:
'third_party/blink/renderer/platform/scheduler/public/scheduling_lifecycle_state.h'
file not found

I changed FileManager.cpp like this:

$ git diff
diff --git a/clang/lib/Basic/FileManager.cpp
b/clang/lib/Basic/FileManager.cpp
index 01acfd5dd61..5388c7f879c 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -460,16 +460,28 @@ FileManager::getBufferForFile(StringRef Filename,
bool isVolatile) {
 /// do directory look-up instead of file look-up.
 bool FileManager::getStatValue(StringRef Path, FileData &Data, bool isFile,
std::unique_ptr *F) {
+bool b = false;
+  if (Path.endswith("scheduling_lifecycle_state.h")) {
+fprintf(stderr, "%s: %s %p\n", Path.str().c_str(),
FS->getCurrentWorkingDirectory().get().c_str(), F);
+b = true;
+  }
+
   // FIXME: FileSystemOpts shouldn't be passed in here, all paths should be
   // absolute!
-  if (FileSystemOpts.WorkingDir.empty())
-return FileSystemStatCache::get(Path, Data, isFile, F,StatCache.get(),
*FS);
+  if (FileSystemOpts.WorkingDir.empty()) {
+bool R =
+FileSystemStatCache::get(Path, Data, isFile, F, StatCache.get(),
*FS);
+if (b) fprintf(stderr, "early get() %d\n", R);
+return R;
+  }

   SmallString<128> FilePath(Path);
   FixupRelativePath(FilePath);

-  return FileSystemStatCache::get(FilePath.c_str(), Data, isFile, F,
-  StatCache.get(), *FS);
+  bool R = FileSystemStatCache::get(FilePath.c_str(), Data, isFile, F,
+StatCache.get(), *FS);
+if (b) fprintf(stderr, "get() %d\n", R);
+  return R;
 }



The output with that patch is:

$ time ./run.sh
pch
main
../../third_party/blink/renderer/platform/scheduler/public/scheduling_lifecycle_state.h:
/Users/thakis/src/chrome/src/out/gnwin 0x7fff5e677150
early get() 1
In file included from
../../third_party/blink/renderer/core/layout/layout_quote.cc:40:
../../third_party/blink/renderer/platform/scheduler/public/frame_or_worker_scheduler.h(10,10):
fatal error:
'third_party/blink/renderer/platform/scheduler/public/scheduling_lifecycle_state.h'
file not found
#include
"third_party/blink/renderer/platform/scheduler/public/scheduling_lifecycle_state.h"



It looks like FS->getCurrentWorkingDirectory() is set
yet FileSystemOpts.WorkingDir.empty() is also true. Is that expected? But
the output doesn't look like you expect, at least.


>
> I can also try to repro locally, though I don't have a mac or experience
> building chromium, and it sounds likely this is at least somewhat
> platform-dependent.
>
> On Wed, Jan 23, 2019 at 4:17 AM Nico Weber via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> I don't have a reduced test case yet, but this seems to cause clang to
>> sometimes claim that an included file isn't found even if it's there, at
>> least on macOS:
>> https://bugs.chromium.org/p/chromium/issues/detail?id=924225
>>
>> On Mon, Nov 19, 2018 at 8:40 AM Sam McCall via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: sammccall
>>> Date: Mon Nov 19 05:37:46 2018
>>> New Revision: 347205
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=347205&view=rev
>>> Log:
>>> [FileManager] getFile(open=true) after getFile(open=false) should open
>>> the file.
>>>
>>> Summary:
>>> Old behavior is to just return the cached entry regardless of
>>> opened-ness.
>>> That feels buggy (though I guess nobody ever actually needed this).
>>>
>>> This came up in the context of clangd

[PATCH] D57093: [CodeComplete] [clangd] Fix crash on ValueDecl with a null type

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

LGTM




Comment at: clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp:2322
 
+TEST(CompletionTest, CrashOnNullType) {
+  auto Results = completions(R"cpp(

nit: WorksWithNullType ? it doesn't seem like crashing


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

https://reviews.llvm.org/D57093



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

[PATCH] D57064: [Sema] Improve a -Warray-bounds diagnostic

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM with a testing request and some small nits.




Comment at: clang/include/clang/AST/ASTContext.h:2092
+  Optional getTypeSizeInCharsIfKnown(QualType Ty) const {
+if (Ty->isIncompleteType() || Ty->isDependentType())
+  return None;

Can you add a dependent type test as well?



Comment at: clang/test/Sema/static-array.c:25
+  cat((int *)d); // expected-warning {{array argument is too small; is of size 
4, callee requires at least 12}}
+  cat(d); // expected-warning {{array argument is too small; is of size 4, 
callee requires at least 12}} expected-warning{{incompatible pointer types}}
+

Add an extra space after `expected-warning` and the `{` for the second warning.



Comment at: clang/test/Sema/static-array.c:29
+  cat((int *)e);
+  cat(e); // expected-warning{{incompatible pointer types}}
 }

Same here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57064



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping :)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added reviewers: aaron.ballman, steveire.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.

Introduce `GenericSelectionExpr::Association` which wraps an association
expression and its `TypeSourceInfo`. Add the boilerplate necessary to use
ranges of `Associations`.

Additionally pack `GenericSelectionExpr` by tail-allocating the array of
selection expressions and `TypeSourceInfo`.

Note that this is just a draft following D56959 
.


Repository:
  rC Clang

https://reviews.llvm.org/D57098

Files:
  include/clang/AST/Expr.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/AST/Stmt.h
  include/clang/AST/StmtDataCollectors.td
  lib/AST/ASTDumper.cpp
  lib/AST/Expr.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/StmtProfile.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaPseudoObject.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp

Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -968,18 +968,21 @@
 
 void ASTStmtWriter::VisitGenericSelectionExpr(GenericSelectionExpr *E) {
   VisitExpr(E);
-  Record.push_back(E->getNumAssocs());
 
-  Record.AddStmt(E->getControllingExpr());
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
-Record.AddTypeSourceInfo(E->getAssocTypeSourceInfo(I));
-Record.AddStmt(E->getAssocExpr(I));
-  }
+  Record.push_back(E->getNumAssocs());
   Record.push_back(E->isResultDependent() ? -1U : E->getResultIndex());
-
   Record.AddSourceLocation(E->getGenericLoc());
   Record.AddSourceLocation(E->getDefaultLoc());
   Record.AddSourceLocation(E->getRParenLoc());
+
+  Stmt **Stmts = E->getTrailingObjects();
+  for (unsigned I = 0, N = E->getNumAssocs() + 1; I < N; ++I)
+Record.AddStmt(Stmts[I]);
+
+  TypeSourceInfo **TSIs = E->getTrailingObjects();
+  for (unsigned I = 0, N = E->getNumAssocs(); I < N; ++I)
+Record.AddTypeSourceInfo(TSIs[I]);
+
   Code = serialization::EXPR_GENERIC_SELECTION;
 }
 
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -1022,21 +1022,22 @@
 
 void ASTStmtReader::VisitGenericSelectionExpr(GenericSelectionExpr *E) {
   VisitExpr(E);
-  E->NumAssocs = Record.readInt();
-  E->AssocTypes = new (Record.getContext()) TypeSourceInfo*[E->NumAssocs];
-  E->SubExprs =
-   new(Record.getContext()) Stmt*[GenericSelectionExpr::END_EXPR+E->NumAssocs];
-
-  E->SubExprs[GenericSelectionExpr::CONTROLLING] = Record.readSubExpr();
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
-E->AssocTypes[I] = GetTypeSourceInfo();
-E->SubExprs[GenericSelectionExpr::END_EXPR+I] = Record.readSubExpr();
-  }
-  E->ResultIndex = Record.readInt();
 
-  E->GenericLoc = ReadSourceLocation();
+  unsigned NumAssocs = Record.readInt();
+  assert((NumAssocs == E->getNumAssocs()) && "Wrong NumAssocs!");
+  E->ResultIndex = Record.readInt();
+  E->GenericSelectionExprBits.GenericLoc = ReadSourceLocation();
   E->DefaultLoc = ReadSourceLocation();
   E->RParenLoc = ReadSourceLocation();
+
+  Stmt **Stmts = E->getTrailingObjects();
+  for (unsigned I = 0, N = NumAssocs + 1; I < N; ++I)
+Stmts[I] = Record.readSubExpr();
+
+  TypeSourceInfo **TSIs = E->getTrailingObjects();
+  for (unsigned I = 0, N = NumAssocs; I < N; ++I)
+TSIs[I] = GetTypeSourceInfo();
+  ;
 }
 
 void ASTStmtReader::VisitPseudoObjectExpr(PseudoObjectExpr *E) {
@@ -2675,7 +2676,9 @@
   break;
 
 case EXPR_GENERIC_SELECTION:
-  S = new (Context) GenericSelectionExpr(Empty);
+  S = GenericSelectionExpr::CreateEmpty(
+  Context,
+  /*NumAssocs=*/Record[ASTStmtReader::NumExprFields]);
   break;
 
 case EXPR_OBJC_STRING_LITERAL:
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -9071,10 +9071,10 @@
 
   SmallVector AssocExprs;
   SmallVector AssocTypes;
-  for (unsigned i = 0; i != E->getNumAssocs(); ++i) {
-TypeSourceInfo *TS = E->getAssocTypeSourceInfo(i);
-if (TS) {
-  TypeSourceInfo *AssocType = getDerived().TransformType(TS);
+  for (GenericSelectionExpr::Association Assoc : E->associations()) {
+TypeSourceInfo *TSI = Assoc.getTypeSourceInfo();
+if (TSI) {
+  TypeSourceInfo *AssocType = getDerived().TransformType(TSI);
   if (!AssocType)
 return ExprError();
   AssocTypes.push_back(AssocType);
@@ -9082,7 +9082,8 @@
   AssocTypes.push_back(nullptr);
 }
 
-ExprResult AssocExpr = getDerived().TransformExpr(E->getAssocExpr(i));
+ExprResult AssocExpr =
+getDerived().TransformExpr(Assoc.getAssoci

[PATCH] D57086: Ignore trailing NullStmts in StmtExprs for GCC compatibility

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman edited reviewers, added: aaron.ballman; removed: lattner.
aaron.ballman added a comment.

Can you also add a test case to `TestMiscStmts()` in 
clang\test\AST\ast-dump-stmt.c that demonstrates we calculate the correct 
resulting type and don't lose the extra null statements from the AST?




Comment at: clang/lib/Parse/ParseStmt.cpp:962
   if (Actions.isCurCompoundStmtAStmtExpr()) {
-// Look to see if the next two tokens close the statement expression;
-// if so, this expression statement is the last statement in a
-// statment expression.
-return Tok.isNot(tok::r_brace) || NextToken().isNot(tok::r_paren);
+// For gcc compatibility we skip past NullStmts
+int LookAhead = 0;

gcc -> GCC

Also, add a full stop to the end of the comment.



Comment at: clang/lib/Parse/ParseStmt.cpp:963
+// For gcc compatibility we skip past NullStmts
+int LookAhead = 0;
+while (GetLookAheadToken(LookAhead).is(tok::semi)) {

This should be `unsigned` rather than `int`.



Comment at: clang/lib/Parse/ParseStmt.cpp:965
+while (GetLookAheadToken(LookAhead).is(tok::semi)) {
+  LookAhead++;
+}

Prefer `++LookHead;` since the result isn't needed.



Comment at: clang/lib/Parse/ParseStmt.cpp:971
+// expression.
+
+return GetLookAheadToken(LookAhead).isNot(tok::r_brace) ||

Remove spurious newline.



Comment at: clang/lib/Sema/SemaExpr.cpp:13324
+  // as the type of the stmtexpr. For GCC compatibility this excludes trailing
+  // NullStmts
   QualType Ty = Context.VoidTy;

Add full stop to the end of the sentence.



Comment at: clang/lib/Sema/SemaExpr.cpp:13328
   if (!Compound->body_empty()) {
-Stmt *LastStmt = Compound->body_back();
+// GCC ignores empty statements at the end of compound expressions
+// i.e. ({ 5;;; })

Add full stop to the end of the sentence.



Comment at: clang/lib/Sema/SemaExpr.cpp:13331
+//   ^^ ignored
+// This code skips past these NullStmts
+Stmt *LastStmt = nullptr;

Add full stop to the end of the sentence.


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

https://reviews.llvm.org/D57086



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

[PATCH] D56959: [AST] NFC: Introduce new class GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: include/clang/AST/Expr.h:5068
 
+  Association getAssociation(unsigned I) const {
+return Association(cast(SubExprs[END_EXPR + I]), AssocTypes[I],

riccibruno wrote:
> aaron.ballman wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > steveire wrote:
> > > > > aaron.ballman wrote:
> > > > > > Rather than gin these objects up on demand every time they're 
> > > > > > needed, I'd prefer to see the class store `Association` objects 
> > > > > > directly. I don't think it will be easy to do that and still 
> > > > > > support `getAssocExprs()` and `getAssocTypeSourceInfos()`, so I 
> > > > > > think those APIs should be removed in favor of this one. There's 
> > > > > > currently not many uses of `getAssocExprs()` or 
> > > > > > `getAssocTypeSourceInfos()` (I spot one each in Clang) so migration 
> > > > > > to the new API should not be onerous.
> > > > > > 
> > > > > > This should also have a range-based accessor version so that users 
> > > > > > aren't required to use iterative loops to access the information (a 
> > > > > > lot of the places you're already touching could use that 
> > > > > > range-based interface).
> > > > > I would prefer that too, but it doesn't seem to be possible. This is 
> > > > > a sub-range of the `SubExprs` returned from `children()`. 
> > > > > 
> > > > > In theory, that could be split into two members, but then you would 
> > > > > need a range library to recombine them and implement `children()`: 
> > > > > https://godbolt.org/z/ZVamdC
> > > > > 
> > > > > This seems to be the best approach for now, and AFAIK it excludes a 
> > > > > range-accessor.
> > > > > I would prefer that too, but it doesn't seem to be possible. This is 
> > > > > a sub-range of the SubExprs returned from children().
> > > > 
> > > > Ugh, you're right. :-(
> > > > 
> > > > > In theory, that could be split into two members, but then you would 
> > > > > need a range library to recombine them and implement children(): 
> > > > > https://godbolt.org/z/ZVamdC
> > > > 
> > > > We have zip iterators that could be used to implement this, I believe. 
> > > > (see STLExtras.h)
> > > > 
> > > > Alternatively, we could tail-allocate the Association objects with 
> > > > (perhaps references to) pointers back into the Expr tail-allocated 
> > > > array. Not ideal, but does provide a clean interface.
> > > > 
> > > > @riccibruno may have other ideas on how to pack the arrays, as he's 
> > > > done a lot of this work recently.
> > > > We have zip iterators that could be used to implement this, I believe.
> > > 
> > > You're right, there is a `concat` there, but on second thought - because 
> > > Association and Stmt don't share a base, I don't think it can work.
> > > 
> > > > Alternatively, we could tail-allocate the Association objects with 
> > > > (perhaps references to) pointers back into the Expr tail-allocated 
> > > > array. Not ideal, but does provide a clean interface.
> > > 
> > > Perhaps this can work, but I don't know how to do it. If you have scope 
> > > for it in your part of the efforts, it would be a good way to get this 
> > > unblocked.
> > > Perhaps this can work, but I don't know how to do it. If you have scope 
> > > for it in your part of the efforts, it would be a good way to get this 
> > > unblocked.
> > 
> > I'll put some time into it today and see where it goes. You may be right 
> > that this is more work than it's worth, so we'll see.
> I don't see what would prevent tail-allocating the array of sub-expression 
> and the array of `TypeSourceInfo`, and removing the `getAssocExpr`, 
> `getAssocTypeSourceInfo`, `getAssocType` interface in favor of a single 
> `getAssociation`. Then create a range version of `getAssociation` using the 
> fact that if you know where you are in the sub-expression array, you know 
> where is the corresponding `TypeSourceInfo`. To know which index correspond 
> to the selected sub-expression you could use one of the low bits in the 
> `TypeSourceInfo` pointers.
> 
> This means that `children` is still simple to implement, and users can use a 
> single unified
> interface via `getAssociation` and the range version `associations()`. From 
> the point of view of the users it would be like if the `Association` objects 
> were stored contiguously.
Made a patch which roughly the above idea: D57098


Repository:
  rC Clang

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

https://reviews.llvm.org/D56959



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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Sam McCall via cfe-commits]

(Email is better than IRC if that's OK - I don't know this code that well
so it takes me a while).

Thanks, that's definitely interesting and not what I expected. I thought
every call sequence r347205 changed the behavior of would have resulted in
two calls to getStatValue().
I guess the "pch"/"main" output is printed before the corresponding lines
in run.sh? Weird that we don't get any output from building the PCH, but I
don't know well how PCH builds work.

> It looks like FS->getCurrentWorkingDirectory() is set
yet FileSystemOpts.WorkingDir.empty() is also true. Is that expected?
I think so. The FileManager and the VFS each have their own concept of
working directory, I guess for historical reasons.
Making use of the VFS one but not the FileManager one seems reasonable.

So the weirdness is that FileSystemStatCache::get() is returning true (i.e.
file doesn't exist), when the file does exist.
Possibilities:
1) we're serving this result from the FileSystemStatCache (and maybe it's
being poisoned in a PCH-related way somehow). Except as far as I can tell,
FileSystemStatCache is always null (FileManager::setStateCache has no
callers).
2) the FS.openFileForRead call failed (ultimately ::open, if FS is the real
FS)
3) the OwnedFile->status() call failed (ultimately ::fstat)
4) I'm misreading the code somehow

Could you find out which of these is going on? Either running in a debugger
or adding some similar printfs to FileSystemStatCache::get() should be
doable.
I'm also going to try to work out how the patch could have affected this,
but new vs correct much easier for me to compare than new vs old...
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is looking great! I've got some comments, mostly nits. Just to 
double-check, have you verified that these changes do not break any existing 
tests (in clang or clang-tools-extra)?




Comment at: include/clang/AST/Expr.h:5026
+  /// result expression in the case where the generic selection expression
+  /// is not result-dependent. The result index is equal to -1u if and only
+  /// if the generic selection expression is result-dependent.

`~0U` instead of `-1u` so it doesn't suggest a weird type mismatch?



Comment at: include/clang/AST/Expr.h:5030
 
-public:
-  GenericSelectionExpr(const ASTContext &Context,
-   SourceLocation GenericLoc, Expr *ControllingExpr,
-   ArrayRef AssocTypes,
-   ArrayRef AssocExprs,
-   SourceLocation DefaultLoc, SourceLocation RParenLoc,
+  /// The location of the "default" and of the right parenthese.
+  SourceLocation DefaultLoc, RParenLoc;

parenthese -> parenthesis



Comment at: include/clang/AST/Expr.h:5034
+  // GenericSelectionExpr is followed by several trailing objects.
+  // They are in order:
+  //

They are (in order):



Comment at: include/clang/AST/Expr.h:5039
+  // * An array of getNumAssocs() TypeSourceInfo *, one for each of the
+  //   association expression.
+  enum { CONTROLLING = 0, ASSOC_EXPR_START = 1 };

expression -> expressions



Comment at: include/clang/AST/Expr.h:5043
+  unsigned numTrailingObjects(OverloadToken) const {
+return 1 + getNumAssocs();
+  }

I think it would be good to put a comment here like "Add one to account for the 
controlling expression; the remainder are the associated expressions."



Comment at: include/clang/AST/Expr.h:5094
+  public:
+using iterator_category = std::forward_iterator_tag;
+using value_type = AssociationTy;

It seems like this should be pretty trivial to make into a random access 
iterator, or am I missing something?



Comment at: include/clang/AST/Expr.h:5097
+using difference_type = std::ptrdiff_t;
+using pointer = AssociationTy;
+using reference = AssociationTy;

Cute, but I suspect this may come back to bite us at some point. For instance, 
if someone thinks they're working with a real pointer, they're likely to expect 
pointer arithmetic to work when it won't (at least they'll get compile errors 
though).



Comment at: include/clang/AST/Expr.h:5101
+AssociationTy operator*() const {
+  return AssociationTy(cast(*E), *TSI,
+  Offset == SelectedOffset);

This should return `reference` instead.



Comment at: include/clang/AST/Expr.h:5104
+}
+AssociationTy operator->() const { return **this; }
+AssociationIteratorTy &operator++() {

This should return `pointer` instead.



Comment at: include/clang/AST/Expr.h:5106-5108
+  E += 1;
+  TSI += 1;
+  Offset += 1;

Use `++Foo` for each of these?



Comment at: include/clang/AST/Expr.h:5186
+  /// Whether this generic selection is result-dependent.
+  bool isResultDependent() const { return ResultIndex == -1U; }
 

`std::numeric_limits::max()`?



Comment at: include/clang/AST/Expr.h:5208
+  ArrayRef getAssocExprs() const {
+return {reinterpret_cast(getTrailingObjects() +
+ASSOC_EXPR_START),

Do we need to use `reinterpret_cast` here, or can this be done through llvm's 
casting machinery instead?



Comment at: include/clang/AST/Expr.h:5219
+  Association getAssociation(unsigned I) {
+assert((I < getNumAssocs()) &&
+   "Out-of-range index in GenericSelectionExpr::getAssociation!");

Spurious parens can be removed.



Comment at: lib/AST/Expr.cpp:3791
+  DefaultLoc(DefaultLoc), RParenLoc(RParenLoc) {
+  assert((AssocTypes.size() == AssocExprs.size()) &&
+ "Must have the same number of association expressions"

Spurious parens.



Comment at: lib/AST/Expr.cpp:3794
+ " and TypeSourceInfo!");
+  assert((ResultIndex < NumAssocs) && "ResultIndex is out-of-bounds!");
+

Spurious parens.



Comment at: lib/AST/Expr.cpp:3816
+  RParenLoc(RParenLoc) {
+  assert((AssocTypes.size() == AssocExprs.size()) &&
+ "Must have the same number of association expressions"

Spurious parens.



Comment at: lib/Sema/SemaExprObjC.cpp:4339
+subTypes.reserve(n);
+for (GenericSelectionExpr::Association Assoc : gse->associations()) {
+  subTypes.push_back(Assoc.getT

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: aaron.ballman, alexfh, hokein, baloghadamsoftware.
Herald added subscribers: cfe-commits, rnkovacs, xazax.hun, mgorny.

The check `bugprone-exception-escape` does an AST-based analysis to determine
if a function might throw an exception and warns based on that information.
The analysis part is refactored into a standalone class similiar to
`ExprMutAnalyzer` that is generally useful.
I intent to use that class in a new check to automatically introduce `noexcept`
if possible.

`bugprone-exception-escape` uses the consistent semicolon-separated list for
configuration now as well.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D57100

Files:
  clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tidy/bugprone/ExceptionEscapeCheck.h
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tidy/utils/ExceptionAnalyzer.h
  test/clang-tidy/bugprone-exception-escape.cpp

Index: test/clang-tidy/bugprone-exception-escape.cpp
===
--- test/clang-tidy/bugprone-exception-escape.cpp
+++ test/clang-tidy/bugprone-exception-escape.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- -extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'}]}" --
+// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- -extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1;ignored2'}, {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1;enabled2;enabled3'}]}" --
 
 struct throwing_destructor {
   ~throwing_destructor() {
Index: clang-tidy/utils/ExceptionAnalyzer.h
===
--- /dev/null
+++ clang-tidy/utils/ExceptionAnalyzer.h
@@ -0,0 +1,50 @@
+//===--- ExceptionAnalyzer.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
+
+#include "clang/AST/ASTContext.h"
+#include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/StringSet.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+/// This class analysis if a `FunctionDecl` can in principle throw an exception,
+/// either directly or indirectly.
+/// It can be configured to ignore some exception types, especially
+/// `std::bad_alloc` can be disabled as using dynamic memory will always
+/// give the possibility of an exception.
+class ExceptionTracer {
+public:
+  ExceptionTracer() = default;
+
+  bool throwsException(const FunctionDecl *Func);
+  void ignoreExceptions(llvm::StringSet<> ExceptionNames) {
+IgnoredExceptions = std::move(ExceptionNames);
+  }
+
+private:
+  using TypeVec = llvm::SmallVector;
+
+  TypeVec throwsException(const FunctionDecl *Func,
+  llvm::SmallSet &CallStack);
+  TypeVec throwsException(const Stmt *St, const TypeVec &Caught,
+  llvm::SmallSet &CallStack);
+  bool isIgnoredExceptionType(const Type* Exception);
+
+  llvm::StringSet<> IgnoredExceptions;
+  llvm::DenseMap FunctionCache;
+};
+} // namespace utils
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
Index: clang-tidy/utils/ExceptionAnalyzer.cpp
===
--- /dev/null
+++ clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -0,0 +1,146 @@
+#include "ExceptionAnalyzer.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+static bool isBaseOf(const Type *DerivedType, const Type *BaseType) {
+  const auto *DerivedClass = DerivedType->getAsCXXRecordDecl();
+  const auto *BaseClass = BaseType->getAsCXXRecordDecl();
+  if (!DerivedClass || !BaseClass)
+return false;
+
+  return !DerivedClass->forallBases(
+  [BaseClass](const CXXRecordDecl *Cur) { return Cur != BaseClass; });
+}
+
+namespace tidy {
+namespace utils {
+
+ExceptionTracer::TypeVec ExceptionTracer::throwsException(
+const FunctionDecl *Func,
+llvm::SmallSet &CallStack) {
+  if (CallStack.count(Func))
+return TypeVec();
+
+  if (const Stmt *Body = Func->getBody()) {
+CallStack.insert(Func);
+const TypeVec Result = throwsException(Body, TypeVec(), CallStack);
+CallStack.erase(Func);
+return Result;
+  }
+
+  TypeVec R

[PATCH] D56160: [clang-tidy] modernize-use-trailing-return check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D56160#1367074 , @bernhardmgruber 
wrote:

> Thank you again @JonasToth for all your valueable input! I could almost 
> successfully run my check on the llvm/lib subfolder. I created a compilation 
> database from within Visual Studio using an extension called SourceTrail 
> .
>  One of the issues was the following:


Very good!

>   struct Object { long long value; };
>   class C {
> int Object;
> struct Object m();
>   };
>   Object C::m() { return {0}; }
> 
> 
> If I rewrite this to the following
> 
>   struct Object { long long value; };
>   class C {
> int Object;
> struct Object m();
>   };
>   auto C::m() -> Object { return {0}; }
> 
> 
> a compilation error occurs afterwards, because Object now refers to the class 
> member. I discovered a similar problem colliding with the name of a function 
> argument. So essentially, I believe I now require a name lookup of the return 
> type (if it is unqualified) in the scope of the function. In case such a name 
> already exists, i have to prefix `struct/class` or perform no replacement. I 
> looked at the documentation of `ASTContext`, but it seems all the good stuff 
> is in `DeclContext`, which I have not available. How can I perform a name 
> lookup inside the `check` member function?

That is very interesting and I was not aware of this possibility :D

Every `Decl` derives from `DeclContext`, see for example the docs for 
`CXXMethodDecl` 
(https://clang.llvm.org/doxygen/classclang_1_1CXXMethodDecl.html)
You should be able to look up the names you are interested. I don't know of a 
good way to check for the issue we have right now, @aaron.ballman knows that 
probably better then I do.
Try to experiment, but If you don't find a solution come back to us, we will 
figure something out (or ask in IRC).


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

https://reviews.llvm.org/D56160



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

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Two passing-by remarks:

1. I would *love* for this check to produce less cryptic reports :) It 
currently does not output any details whatsoever. It's really unhelpful.
2. It would be great for it to be generalized to not only detect the escaping 
of exceptions out of functions, but out of "statements" too. Namely, i would 
love some better handling of openmp directives. I might look into that later.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100



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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

On Wed, Jan 23, 2019 at 9:54 AM Sam McCall  wrote:

> (Email is better than IRC if that's OK - I don't know this code that well
> so it takes me a while).
>
> Thanks, that's definitely interesting and not what I expected. I thought
> every call sequence r347205 changed the behavior of would have resulted in
> two calls to getStatValue().
> I guess the "pch"/"main" output is printed before the corresponding lines
> in run.sh?
>

Correct.


> Weird that we don't get any output from building the PCH, but I don't know
> well how PCH builds work.
>
> > It looks like FS->getCurrentWorkingDirectory() is set
> yet FileSystemOpts.WorkingDir.empty() is also true. Is that expected?
> I think so. The FileManager and the VFS each have their own concept of
> working directory, I guess for historical reasons.
> Making use of the VFS one but not the FileManager one seems reasonable.
>
> So the weirdness is that FileSystemStatCache::get() is returning true
> (i.e. file doesn't exist), when the file does exist.
> Possibilities:
> 1) we're serving this result from the FileSystemStatCache (and maybe it's
> being poisoned in a PCH-related way somehow). Except as far as I can tell,
> FileSystemStatCache is always null (FileManager::setStateCache has no
> callers).
> 2) the FS.openFileForRead call failed (ultimately ::open, if FS is the
> real FS)
> 3) the OwnedFile->status() call failed (ultimately ::fstat)
> 4) I'm misreading the code somehow
>

::open() fails with errno == 24, EMFILE.

This log statement here:

diff --git a/clang/lib/Basic/FileSystemStatCache.cpp
b/clang/lib/Basic/FileSystemStatCache.cpp
index d29ebb750fc..63fc4780237 100644
--- a/clang/lib/Basic/FileSystemStatCache.cpp
+++ b/clang/lib/Basic/FileSystemStatCache.cpp
@@ -70,9 +70,13 @@ bool FileSystemStatCache::get(StringRef Path, FileData
&Data, bool isFile,
 //
 // Because of this, check to see if the file exists with 'open'.  If
the
 // open succeeds, use fstat to get the stat info.
-auto OwnedFile = FS.openFileForRead(Path);
+llvm::ErrorOr> OwnedFile =
+FS.openFileForRead(Path);

 if (!OwnedFile) {
+if (Path.endswith("scheduling_lifecycle_state.h")) {
+fprintf(stderr, "hmm failed %s\n", OwnedFile.getError().message().c_str());
+}
   // If the open fails, our "stat" fails.
   R = CacheMissing;
 } else {


causes clang to print "hmm failed Too many open files". That path should
maybe check if `OwnedFile.getError().value() == EMFILE &&
OwnedFile.getError().category() == std::generic_category()` on mac and
abort or diag or something more visible.

`ulimit -n` on macOS is pretty small -- do you see how your patch could
cause clang to keep more file handles open?

Here's how many files clang had open when I had a breakpoint in that error
path:

$ lsof -p 91890 | wc -l
 343



>
> Could you find out which of these is going on? Either running in a
> debugger or adding some similar printfs to FileSystemStatCache::get()
> should be doable.
> I'm also going to try to work out how the patch could have affected this,
> but new vs correct much easier for me to compare than new vs old...
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57012: Merge similar target diagnostics for interrupt attribute into one. NFC

[Kristina Bessonova via Phabricator via cfe-commits]

krisb added a comment.

@aaron.ballman yes and yes. Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57012



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

[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Thanks for doing this!

It would be easier to review (now and in the future!) if you split it into at 
least 3 commits

- Refactor without changing API to use trailing objects
- The change to use bitfields and Stmt.h
- Introduce new class GenericSelectionExpr::Association

Thanks,

Stephen


Repository:
  rC Clang

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

https://reviews.llvm.org/D57098



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

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D57100#1367813 , @lebedev.ri wrote:

> Two passing-by remarks:
>
> 1. I would *love* for this check to produce less cryptic reports :) It 
> currently does not output any details whatsoever. It's really unhelpful.


I would do that in a second refactoring. As it is now possible to have state 
within the analysis it should be possible to extract a path that lead to the 
exception. I think the 'highest' throwing function should be noted as the 
source.

  void indirect_throw() { call_something_throwing(); }
  void diagnose() noexcept {
  // warning: this function can throw
  
indirect_throw();
// note: this function is not safe to call
  }



> 2. It would be great for it to be generalized to not only detect the escaping 
> of exceptions out of functions, but out of "statements" too. Namely, i would 
> love some better handling of openmp directives. I might look into that later.

The internals can handle `Stmt` already and do traverse all statements of a 
`FunctionDecl`, so it should be possible to expose that.
Can OpenMP directives throw?

I think making this class and its interface is the first step to both features, 
incremental improvements are then easier to do.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100



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

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 183099.
JonasToth removed a subscriber: lebedev.ri.
JonasToth added a comment.

- adjust doc to say semicolon separated list instead of comma


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100

Files:
  clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tidy/bugprone/ExceptionEscapeCheck.h
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tidy/utils/ExceptionAnalyzer.h
  docs/clang-tidy/checks/bugprone-exception-escape.rst
  test/clang-tidy/bugprone-exception-escape.cpp

Index: test/clang-tidy/bugprone-exception-escape.cpp
===
--- test/clang-tidy/bugprone-exception-escape.cpp
+++ test/clang-tidy/bugprone-exception-escape.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- -extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'}]}" --
+// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- -extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1;ignored2'}, {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1;enabled2;enabled3'}]}" --
 
 struct throwing_destructor {
   ~throwing_destructor() {
Index: docs/clang-tidy/checks/bugprone-exception-escape.rst
===
--- docs/clang-tidy/checks/bugprone-exception-escape.rst
+++ docs/clang-tidy/checks/bugprone-exception-escape.rst
@@ -28,12 +28,12 @@
 
 .. option:: FunctionsThatShouldNotThrow
 
-   Comma separated list containing function names which should not throw. An
+   Semicolon separated list containing function names which should not throw. An
example value for this parameter can be ``WinMain`` which adds function
``WinMain()`` in the Windows API to the list of the funcions which should
not throw. Default value is an empty string.
 
 .. option:: IgnoredExceptions
 
-   Comma separated list containing type names which are not counted as thrown
+   Semicolon separated list containing type names which are not counted as thrown
exceptions in the check. Default value is an empty string.
Index: clang-tidy/utils/ExceptionAnalyzer.h
===
--- /dev/null
+++ clang-tidy/utils/ExceptionAnalyzer.h
@@ -0,0 +1,50 @@
+//===--- ExceptionAnalyzer.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
+
+#include "clang/AST/ASTContext.h"
+#include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/StringSet.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+/// This class analysis if a `FunctionDecl` can in principle throw an exception,
+/// either directly or indirectly.
+/// It can be configured to ignore some exception types, especially
+/// `std::bad_alloc` can be disabled as using dynamic memory will always
+/// give the possibility of an exception.
+class ExceptionTracer {
+public:
+  ExceptionTracer() = default;
+
+  bool throwsException(const FunctionDecl *Func);
+  void ignoreExceptions(llvm::StringSet<> ExceptionNames) {
+IgnoredExceptions = std::move(ExceptionNames);
+  }
+
+private:
+  using TypeVec = llvm::SmallVector;
+
+  TypeVec throwsException(const FunctionDecl *Func,
+  llvm::SmallSet &CallStack);
+  TypeVec throwsException(const Stmt *St, const TypeVec &Caught,
+  llvm::SmallSet &CallStack);
+  bool isIgnoredExceptionType(const Type* Exception);
+
+  llvm::StringSet<> IgnoredExceptions;
+  llvm::DenseMap FunctionCache;
+};
+} // namespace utils
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
Index: clang-tidy/utils/ExceptionAnalyzer.cpp
===
--- /dev/null
+++ clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -0,0 +1,146 @@
+#include "ExceptionAnalyzer.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+static bool isBaseOf(const Type *DerivedType, const Type *BaseType) {
+  const auto *DerivedClass = DerivedType->getAsCXXRecordDecl();
+  const auto *BaseClass = BaseType->getAsCXXRecordDecl();
+  if (!DerivedClass

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

With your patch reverted locally, at the same breakpoint I instead get

$ lsof -p 95842 | wc -l
  94

So your patch seems to increase number of open file handles by ~260%.

On Wed, Jan 23, 2019 at 10:27 AM Nico Weber  wrote:

> On Wed, Jan 23, 2019 at 9:54 AM Sam McCall  wrote:
>
>> (Email is better than IRC if that's OK - I don't know this code that well
>> so it takes me a while).
>>
>> Thanks, that's definitely interesting and not what I expected. I thought
>> every call sequence r347205 changed the behavior of would have resulted in
>> two calls to getStatValue().
>> I guess the "pch"/"main" output is printed before the corresponding lines
>> in run.sh?
>>
>
> Correct.
>
>
>> Weird that we don't get any output from building the PCH, but I don't
>> know well how PCH builds work.
>>
>> > It looks like FS->getCurrentWorkingDirectory() is set
>> yet FileSystemOpts.WorkingDir.empty() is also true. Is that expected?
>> I think so. The FileManager and the VFS each have their own concept of
>> working directory, I guess for historical reasons.
>> Making use of the VFS one but not the FileManager one seems reasonable.
>>
>> So the weirdness is that FileSystemStatCache::get() is returning true
>> (i.e. file doesn't exist), when the file does exist.
>> Possibilities:
>> 1) we're serving this result from the FileSystemStatCache (and maybe it's
>> being poisoned in a PCH-related way somehow). Except as far as I can tell,
>> FileSystemStatCache is always null (FileManager::setStateCache has no
>> callers).
>> 2) the FS.openFileForRead call failed (ultimately ::open, if FS is the
>> real FS)
>> 3) the OwnedFile->status() call failed (ultimately ::fstat)
>> 4) I'm misreading the code somehow
>>
>
> ::open() fails with errno == 24, EMFILE.
>
> This log statement here:
>
> diff --git a/clang/lib/Basic/FileSystemStatCache.cpp
> b/clang/lib/Basic/FileSystemStatCache.cpp
> index d29ebb750fc..63fc4780237 100644
> --- a/clang/lib/Basic/FileSystemStatCache.cpp
> +++ b/clang/lib/Basic/FileSystemStatCache.cpp
> @@ -70,9 +70,13 @@ bool FileSystemStatCache::get(StringRef Path, FileData
> &Data, bool isFile,
>  //
>  // Because of this, check to see if the file exists with 'open'.  If
> the
>  // open succeeds, use fstat to get the stat info.
> -auto OwnedFile = FS.openFileForRead(Path);
> +llvm::ErrorOr> OwnedFile =
> +FS.openFileForRead(Path);
>
>  if (!OwnedFile) {
> +if (Path.endswith("scheduling_lifecycle_state.h")) {
> +fprintf(stderr, "hmm failed %s\n",
> OwnedFile.getError().message().c_str());
> +}
>// If the open fails, our "stat" fails.
>R = CacheMissing;
>  } else {
>
>
> causes clang to print "hmm failed Too many open files". That path should
> maybe check if `OwnedFile.getError().value() == EMFILE &&
> OwnedFile.getError().category() == std::generic_category()` on mac and
> abort or diag or something more visible.
>
> `ulimit -n` on macOS is pretty small -- do you see how your patch could
> cause clang to keep more file handles open?
>
> Here's how many files clang had open when I had a breakpoint in that error
> path:
>
> $ lsof -p 91890 | wc -l
>  343
>
>
>
>>
>> Could you find out which of these is going on? Either running in a
>> debugger or adding some similar printfs to FileSystemStatCache::get()
>> should be doable.
>> I'm also going to try to work out how the patch could have affected this,
>> but new vs correct much easier for me to compare than new vs old...
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D57100#1367817 , @JonasToth wrote:

> In D57100#1367813 , @lebedev.ri 
> wrote:
>
> > 2. It would be great for it to be generalized to not only detect the 
> > escaping of exceptions out of functions, but out of "statements" too. 
> > Namely, i would love some better handling of openmp directives. I might 
> > look into that later.
>
>
> The internals can handle `Stmt` already and do traverse all statements of a 
> `FunctionDecl`, so it should be possible to expose that.
>  Can OpenMP directives throw?


The other way around, it's **the** same problem as with exceptions escaping 
noexcept functions,
but just with narrower scope - a statement - as opposed to whole function

> I think making this class and its interface is the first step to both 
> features, incremental improvements are then easier to do.

Yep, don't want to drag anything sideways, just voicing some thoughts.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100



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

[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked 13 inline comments as done.
riccibruno added a comment.

In D57098#1367769 , @aaron.ballman 
wrote:

> This is looking great! I've got some comments, mostly nits. Just to 
> double-check, have you verified that these changes do not break any existing 
> tests (in clang or clang-tools-extra)?


I ran the usual assert build/asan+assert build/msan+assert build.
To be clear I know that there are some changes required but I just wanted to 
put it out here to get
some feedback on the basic idea (which is to use some kind of proxy iterator).

Some added comments inline.




Comment at: include/clang/AST/Expr.h:5026
+  /// result expression in the case where the generic selection expression
+  /// is not result-dependent. The result index is equal to -1u if and only
+  /// if the generic selection expression is result-dependent.

aaron.ballman wrote:
> `~0U` instead of `-1u` so it doesn't suggest a weird type mismatch?
Yes indeed (or `std::numeric_limits::max()` as suggested below).



Comment at: include/clang/AST/Expr.h:5043
+  unsigned numTrailingObjects(OverloadToken) const {
+return 1 + getNumAssocs();
+  }

aaron.ballman wrote:
> I think it would be good to put a comment here like "Add one to account for 
> the controlling expression; the remainder are the associated expressions."
Will do.



Comment at: include/clang/AST/Expr.h:5094
+  public:
+using iterator_category = std::forward_iterator_tag;
+using value_type = AssociationTy;

aaron.ballman wrote:
> It seems like this should be pretty trivial to make into a random access 
> iterator, or am I missing something?
Indeed, at the cost of some boilerplate. I can totally do this but from what I 
understood the motivation was to use this in ranges, and for this a forward 
iterator is good enough (although see the next comment)



Comment at: include/clang/AST/Expr.h:5097
+using difference_type = std::ptrdiff_t;
+using pointer = AssociationTy;
+using reference = AssociationTy;

aaron.ballman wrote:
> Cute, but I suspect this may come back to bite us at some point. For 
> instance, if someone thinks they're working with a real pointer, they're 
> likely to expect pointer arithmetic to work when it won't (at least they'll 
> get compile errors though).
Hmm, but `pointer` is just the return type of `operator->` no ? Is it actually 
required to behave like a pointer ? The only requirement I can find is that 
`It->x` must be equivalent to `(*It).x`, which is true here.

Also looking at the requirements for forward iterators I think that this 
iterator should actually be downgraded to an input iterator, because of the 
requirement that `reference = T&`.



Comment at: include/clang/AST/Expr.h:5101
+AssociationTy operator*() const {
+  return AssociationTy(cast(*E), *TSI,
+  Offset == SelectedOffset);

aaron.ballman wrote:
> This should return `reference` instead.
yes



Comment at: include/clang/AST/Expr.h:5104
+}
+AssociationTy operator->() const { return **this; }
+AssociationIteratorTy &operator++() {

aaron.ballman wrote:
> This should return `pointer` instead.
yes



Comment at: include/clang/AST/Expr.h:5108
+  TSI += 1;
+  Offset += 1;
+  return *this;

yes



Comment at: include/clang/AST/Expr.h:5186
+  /// Whether this generic selection is result-dependent.
+  bool isResultDependent() const { return ResultIndex == -1U; }
 

aaron.ballman wrote:
> `std::numeric_limits::max()`?
yes



Comment at: include/clang/AST/Expr.h:5208
+  ArrayRef getAssocExprs() const {
+return {reinterpret_cast(getTrailingObjects() +
+ASSOC_EXPR_START),

aaron.ballman wrote:
> Do we need to use `reinterpret_cast` here, or can this be done through llvm's 
> casting machinery instead?
I believe that it is needed, because `Stmt **` is not otherwise convertible to 
`Expr **`. This is all over the place in the statement/expression nodes, and 
indeed it depends on the layout of `Expr` and `Stmt`.



Comment at: include/clang/AST/Expr.h:5219
+  Association getAssociation(unsigned I) {
+assert((I < getNumAssocs()) &&
+   "Out-of-range index in GenericSelectionExpr::getAssociation!");

aaron.ballman wrote:
> Spurious parens can be removed.
From the precedence rules yes, but I thought that some gcc bots were
warning about this (or maybe I am mistaken and this is in an other situation) 



Comment at: lib/Sema/SemaPseudoObject.cpp:148
+
+for (GenericSelectionExpr::Association Assoc : gse->associations()) {
+  Expr *assoc = Assoc.getAssociationExpr(

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:133
+StringRef Correct =
+HasFinal ? "'" + FinalMacro + "'" : "'" + OverrideMacro + "'";
 

Dangling? These values seem to be temporary and `StringRef` would bind to the 
temporary, not?
For the concatenation `llvm::Twine` would be better as well, same in the other 
places.


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

https://reviews.llvm.org/D57087



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

[PATCH] D56916: Fix crash due to ObjCPropertyDecl

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

In D56916#1367559 , @ilya-biryukov 
wrote:

> It looks like `Container::_magic` is a platform-dependent completion, I don't 
> have it on Linux, but 
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/42665
>  fails because it's not in the list.
>  Submitted rL351943  to workaround the 
> failure. @dgoldman, any idea why completion might be there on some platforms, 
> but not the others?


Perhaps property auto-synthesis isn't enabled there? I wonder if the following 
manual synthesis would help:

  @implementation Container
  @synthesize magic;
  @end


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56916



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

[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/Expr.h:5208
+  ArrayRef getAssocExprs() const {
+return {reinterpret_cast(getTrailingObjects() +
+ASSOC_EXPR_START),

riccibruno wrote:
> aaron.ballman wrote:
> > Do we need to use `reinterpret_cast` here, or can this be done through 
> > llvm's casting machinery instead?
> I believe that it is needed, because `Stmt **` is not otherwise convertible 
> to `Expr **`. This is all over the place in the statement/expression nodes, 
> and indeed it depends on the layout of `Expr` and `Stmt`.
Ugh ignore the comment about the layout. I was thinking about the 
`reinterpret_cast`s between `Stmt *` and `Expr *` in `Stmt.h`


Repository:
  rC Clang

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

https://reviews.llvm.org/D57098



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

[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D57098#1367816 , @steveire wrote:

> Thanks for doing this!
>
> It would be easier to review (now and in the future!) if you split it into at 
> least 3 commits
>
> - Refactor use trailing objects without introducing the Association class
> - The change to use bitfields and Stmt.h
> - Introduce new class GenericSelectionExpr::Association
>
>   Thanks,
>
>   Stephen


Is it okay if I split it into:

1. `llvm::TrailingObjects` + bit-fields of `Stmt`
2. Introduce `GenericSelectionExpr::Association`

since using `llvm::TrailingObjects` and using the bit-fields of `Stmt` are 
closely related
and come under the title "[AST] Pack GenericSelectionExpr".


Repository:
  rC Clang

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

https://reviews.llvm.org/D57098



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

[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D57098#1367852 , @riccibruno wrote:

> In D57098#1367816 , @steveire wrote:
>
> > Thanks for doing this!
> >
> > It would be easier to review (now and in the future!) if you split it into 
> > at least 3 commits
> >
> > - Refactor use trailing objects without introducing the Association class
> > - The change to use bitfields and Stmt.h
> > - Introduce new class GenericSelectionExpr::Association
> >
> >   Thanks,
> >
> >   Stephen
>
>
> Is it okay if I split it into:
>
> 1. `llvm::TrailingObjects` + bit-fields of `Stmt`
> 2. Introduce `GenericSelectionExpr::Association`
>
>   since using `llvm::TrailingObjects` and using the bit-fields of `Stmt` are 
> closely related and come under the title "[AST] Pack GenericSelectionExpr".


I didn't realize that, so your proposal sounds good to me!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57098



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

Re: [clang-tools-extra] r351788 - [clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB.

[Hans Wennborg via cfe-commits]

Thanks for the info!

On Wed, Jan 23, 2019 at 2:24 AM Kadir Çetinkaya  wrote:
>
> Hey Hans,
>
> There is actually a parent for this patch at 
> https://reviews.llvm.org/rC351531 and it hasn't been merged.

I've merged it now in r351961.

>
> In addition to that not exactly as a follow-up, but there is 
> https://reviews.llvm.org/rCTE351738 which is kind of related to this two 
> patches, you might also consider merging that but it is more of a NFC.

Merged that in r351962.

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

[PATCH] D57101: [AST] Fix addr space of result type for dereference operator

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: rjmccall.

When we dereference a pointer the result of the dereference operation is no 
longer in the address space of its pointee i.e.

  __attribute__((address_space(1))) int  * a;
  (*a);// the expression is in the default addr space and not in addr space 1. 


https://reviews.llvm.org/D57101

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaOpenCLCXX/address_space_overloading.cl


Index: test/SemaOpenCLCXX/address_space_overloading.cl
===
--- test/SemaOpenCLCXX/address_space_overloading.cl
+++ test/SemaOpenCLCXX/address_space_overloading.cl
@@ -1,12 +1,12 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
 
-// FIXME: This test shouldn't trigger any errors.
+// expected-no-diagnostics 
 
 struct RetGlob {
   int dummy;
 };
 
-struct RetGen { //expected-error{{binding value of type '__generic RetGen' to 
reference to type 'RetGen' drops <> qualifiers}}
+struct RetGen {
   char dummy;
 };
 
@@ -19,5 +19,5 @@
   __local int *ArgLoc;
   RetGlob TestGlob = foo(ArgGlob);
   RetGen TestGen = foo(ArgGen);
-  TestGen = foo(ArgLoc); //expected-note{{in implicit copy assignment operator 
for 'RetGen' first required here}}
+  TestGen = foo(ArgLoc);
 }
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12997,6 +12997,8 @@
 Input = DefaultFunctionArrayLvalueConversion(Input.get());
 if (Input.isInvalid()) return ExprError();
 resultType = CheckIndirectionOperand(*this, Input.get(), VK, OpLoc);
+// The result type of a dereferenced object is in the default addr space.
+resultType = Context.removeAddrSpaceQualType(resultType);
 break;
   }
   case UO_Plus:


Index: test/SemaOpenCLCXX/address_space_overloading.cl
===
--- test/SemaOpenCLCXX/address_space_overloading.cl
+++ test/SemaOpenCLCXX/address_space_overloading.cl
@@ -1,12 +1,12 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
 
-// FIXME: This test shouldn't trigger any errors.
+// expected-no-diagnostics 
 
 struct RetGlob {
   int dummy;
 };
 
-struct RetGen { //expected-error{{binding value of type '__generic RetGen' to reference to type 'RetGen' drops <> qualifiers}}
+struct RetGen {
   char dummy;
 };
 
@@ -19,5 +19,5 @@
   __local int *ArgLoc;
   RetGlob TestGlob = foo(ArgGlob);
   RetGen TestGen = foo(ArgGen);
-  TestGen = foo(ArgLoc); //expected-note{{in implicit copy assignment operator for 'RetGen' first required here}}
+  TestGen = foo(ArgLoc);
 }
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12997,6 +12997,8 @@
 Input = DefaultFunctionArrayLvalueConversion(Input.get());
 if (Input.isInvalid()) return ExprError();
 resultType = CheckIndirectionOperand(*this, Input.get(), VK, OpLoc);
+// The result type of a dereferenced object is in the default addr space.
+resultType = Context.removeAddrSpaceQualType(resultType);
 break;
   }
   case UO_Plus:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57101: [AST] Fix addr space of result type for dereference operator

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 183108.
Anastasia added a comment.

- Minot update to the comment.


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

https://reviews.llvm.org/D57101

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaOpenCLCXX/address_space_overloading.cl


Index: test/SemaOpenCLCXX/address_space_overloading.cl
===
--- test/SemaOpenCLCXX/address_space_overloading.cl
+++ test/SemaOpenCLCXX/address_space_overloading.cl
@@ -1,12 +1,12 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
 
-// FIXME: This test shouldn't trigger any errors.
+// expected-no-diagnostics 
 
 struct RetGlob {
   int dummy;
 };
 
-struct RetGen { //expected-error{{binding value of type '__generic RetGen' to 
reference to type 'RetGen' drops <> qualifiers}}
+struct RetGen {
   char dummy;
 };
 
@@ -19,5 +19,5 @@
   __local int *ArgLoc;
   RetGlob TestGlob = foo(ArgGlob);
   RetGen TestGen = foo(ArgGen);
-  TestGen = foo(ArgLoc); //expected-note{{in implicit copy assignment operator 
for 'RetGen' first required here}}
+  TestGen = foo(ArgLoc);
 }
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12997,6 +12997,8 @@
 Input = DefaultFunctionArrayLvalueConversion(Input.get());
 if (Input.isInvalid()) return ExprError();
 resultType = CheckIndirectionOperand(*this, Input.get(), VK, OpLoc);
+// The result type of a dereferenced pointer is in the default addr space.
+resultType = Context.removeAddrSpaceQualType(resultType);
 break;
   }
   case UO_Plus:


Index: test/SemaOpenCLCXX/address_space_overloading.cl
===
--- test/SemaOpenCLCXX/address_space_overloading.cl
+++ test/SemaOpenCLCXX/address_space_overloading.cl
@@ -1,12 +1,12 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
 
-// FIXME: This test shouldn't trigger any errors.
+// expected-no-diagnostics 
 
 struct RetGlob {
   int dummy;
 };
 
-struct RetGen { //expected-error{{binding value of type '__generic RetGen' to reference to type 'RetGen' drops <> qualifiers}}
+struct RetGen {
   char dummy;
 };
 
@@ -19,5 +19,5 @@
   __local int *ArgLoc;
   RetGlob TestGlob = foo(ArgGlob);
   RetGen TestGen = foo(ArgGen);
-  TestGen = foo(ArgLoc); //expected-note{{in implicit copy assignment operator for 'RetGen' first required here}}
+  TestGen = foo(ArgLoc);
 }
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12997,6 +12997,8 @@
 Input = DefaultFunctionArrayLvalueConversion(Input.get());
 if (Input.isInvalid()) return ExprError();
 resultType = CheckIndirectionOperand(*this, Input.get(), VK, OpLoc);
+// The result type of a dereferenced pointer is in the default addr space.
+resultType = Context.removeAddrSpaceQualType(resultType);
 break;
   }
   case UO_Plus:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Sam McCall via cfe-commits]

Thanks! given that we don't see an earlier stat, I guess these files were
being treated as virtual (file metadata deserialized from PCH). Previously
despite the open=true these never actually got opened, and that worked fine.

I'm away from my computer but will verify later tonight or in the morning
(CET) and try to find a fix. If it's not obvious, we should revert the
patch at least on the release branch.

A stack trace at the relevant breakpoint might well be useful - can't
remember if there are lots of entry points here.

Cheers, Sam

On Wed, Jan 23, 2019, 16:38 Nico Weber  With your patch reverted locally, at the same breakpoint I instead get
>
> $ lsof -p 95842 | wc -l
>   94
>
> So your patch seems to increase number of open file handles by ~260%.
>
> On Wed, Jan 23, 2019 at 10:27 AM Nico Weber  wrote:
>
>> On Wed, Jan 23, 2019 at 9:54 AM Sam McCall  wrote:
>>
>>> (Email is better than IRC if that's OK - I don't know this code that
>>> well so it takes me a while).
>>>
>>> Thanks, that's definitely interesting and not what I expected. I thought
>>> every call sequence r347205 changed the behavior of would have resulted in
>>> two calls to getStatValue().
>>> I guess the "pch"/"main" output is printed before the corresponding
>>> lines in run.sh?
>>>
>>
>> Correct.
>>
>>
>>> Weird that we don't get any output from building the PCH, but I don't
>>> know well how PCH builds work.
>>>
>>> > It looks like FS->getCurrentWorkingDirectory() is set
>>> yet FileSystemOpts.WorkingDir.empty() is also true. Is that expected?
>>> I think so. The FileManager and the VFS each have their own concept of
>>> working directory, I guess for historical reasons.
>>> Making use of the VFS one but not the FileManager one seems reasonable.
>>>
>>> So the weirdness is that FileSystemStatCache::get() is returning true
>>> (i.e. file doesn't exist), when the file does exist.
>>> Possibilities:
>>> 1) we're serving this result from the FileSystemStatCache (and maybe
>>> it's being poisoned in a PCH-related way somehow). Except as far as I can
>>> tell, FileSystemStatCache is always null (FileManager::setStateCache has no
>>> callers).
>>> 2) the FS.openFileForRead call failed (ultimately ::open, if FS is the
>>> real FS)
>>> 3) the OwnedFile->status() call failed (ultimately ::fstat)
>>> 4) I'm misreading the code somehow
>>>
>>
>> ::open() fails with errno == 24, EMFILE.
>>
>> This log statement here:
>>
>> diff --git a/clang/lib/Basic/FileSystemStatCache.cpp
>> b/clang/lib/Basic/FileSystemStatCache.cpp
>> index d29ebb750fc..63fc4780237 100644
>> --- a/clang/lib/Basic/FileSystemStatCache.cpp
>> +++ b/clang/lib/Basic/FileSystemStatCache.cpp
>> @@ -70,9 +70,13 @@ bool FileSystemStatCache::get(StringRef Path, FileData
>> &Data, bool isFile,
>>  //
>>  // Because of this, check to see if the file exists with 'open'.  If
>> the
>>  // open succeeds, use fstat to get the stat info.
>> -auto OwnedFile = FS.openFileForRead(Path);
>> +llvm::ErrorOr> OwnedFile =
>> +FS.openFileForRead(Path);
>>
>>  if (!OwnedFile) {
>> +if (Path.endswith("scheduling_lifecycle_state.h")) {
>> +fprintf(stderr, "hmm failed %s\n",
>> OwnedFile.getError().message().c_str());
>> +}
>>// If the open fails, our "stat" fails.
>>R = CacheMissing;
>>  } else {
>>
>>
>> causes clang to print "hmm failed Too many open files". That path should
>> maybe check if `OwnedFile.getError().value() == EMFILE &&
>> OwnedFile.getError().category() == std::generic_category()` on mac and
>> abort or diag or something more visible.
>>
>> `ulimit -n` on macOS is pretty small -- do you see how your patch could
>> cause clang to keep more file handles open?
>>
>> Here's how many files clang had open when I had a breakpoint in that
>> error path:
>>
>> $ lsof -p 91890 | wc -l
>>  343
>>
>>
>>
>>>
>>> Could you find out which of these is going on? Either running in a
>>> debugger or adding some similar printfs to FileSystemStatCache::get()
>>> should be doable.
>>> I'm also going to try to work out how the patch could have affected
>>> this, but new vs correct much easier for me to compare than new vs old...
>>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56760: Add a new builtin: __builtin_dynamic_object_size

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I pointed out this review to Jonathan Wakely, who posted it to the GCC list 
. Jakub replied with:

> The current modes (0-3) certainly must not be changed and must return a
>  constant, that is what huge amounts of code in the wild relies on.
> 
> The reason to choose constants only was the desire to make `_FORTIFY_SOURCE`
>  cheap at runtime.  For the dynamically computed expressions, the question
>  is how far it should go, how complex expressions it wants to build and how
>  much code and runtime can be spent on computing that.
> 
> The rationale for `__builtin_dynamic_object_size` lists only very simple
>  cases, where the builtin is just called on result of malloc, so that is
>  indeed easy, the argument is already evaluated before the malloc call, so
>  you can just save it in a temporary and use later.  Slightly more complex
>  is calloc, where you need to multiply two numbers (do you handle overflow
>  somehow, or not?).  But in real world, it can be arbitrarily more complex,
>  there can be pointer arithmetics with constant or variable offsets,
>  there can be conditional adjustments of pointers or PHIs with multiple
>  "dynamic" expressions for the sizes (shall the dynamic expression evaluate
>  as max over expressions for different phi arguments (that is essentially
>  what is done for the constant `__builtin_object_size`, but for dynamic
>  expressions max needs to be computed at runtime, or shall it reconstruct
>  the conditional or remember it and compute `whatever ? val1 : val2`),
>  loops which adjust pointers, etc. and all that can be done many times in
>  between where the objects are allocated and where the builtin is used.

@rsmith, what do you think and how do you want to proceed? We think something 
like what Erik implemented will catch things `_FORTIFY_SOURCE` currently 
cannot. We agree there are valid code generation complexity concerns, yet it 
seems like having a different spelling for the builtin helps mitigate those 
concerns.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56760



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

[PATCH] D56003: [RFC] [CFE] Allocatable Global Register Variables for ARM

[Carey Williams via Phabricator via cfe-commits]

carwil updated this revision to Diff 183114.
carwil added a comment.

More tests, and better handling of argument combination errors.


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

https://reviews.llvm.org/D56003

Files:
  docs/ClangCommandLineReference.rst
  include/clang/Driver/Options.td
  lib/Basic/Targets/ARM.cpp
  lib/Basic/Targets/ARM.h
  lib/Driver/ToolChains/Arch/ARM.cpp
  test/Driver/arm-reserved-reg-options.c
  test/Sema/arm-global-regs.c

Index: test/Sema/arm-global-regs.c
===
--- /dev/null
+++ test/Sema/arm-global-regs.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -ffreestanding -fsyntax-only -verify -triple arm %s
+
+// Check a small subset of valid and invalid global register variable declarations.
+
+register unsigned arm_r3 __asm("r3");   //expected-error {{register 'r3' unsuitable for global register variables on this target}}
+
+register unsigned arm_r12 __asm("r12"); //expected-error {{register 'r12' unsuitable for global register variables on this target}}
+
+register unsigned arm_r5 __asm("r5");
+
+register unsigned arm_r9 __asm("r9");
+
+register unsigned arm_sp __asm("sp");
\ No newline at end of file
Index: test/Driver/arm-reserved-reg-options.c
===
--- /dev/null
+++ test/Driver/arm-reserved-reg-options.c
@@ -0,0 +1,68 @@
+// ## FP ARM + Thumb
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITHOUT-OMIT %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITH-NO-OMIT %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -fno-omit-frame-pointer -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -fomit-frame-pointer -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITH-NO-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITHOUT-OMIT %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-NO-OMIT %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -fno-omit-frame-pointer -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -fomit-frame-pointer -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-NO-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -mthumb -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST %s
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -fomit-frame-pointer -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r7 -fno-omit-frame-pointer -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -mthumb -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -mthumb -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -mthumb -fomit-frame-pointer -fno-omit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+// RUN: %clang -target arm-arm-none-eabi -### -ffixed-r11 -mthumb -fno-omit-frame-pointer -fomit-frame-pointer -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITH-OMIT-LAST -check-prefix=CHECK-RESERVED-R11-WITH-OMIT-LAST %s
+
+// RUN: %clang -target thumbv6m-none-eabi -### -ffixed-r7 -c %s 2>&1 | FileCheck -check-prefix=CHECK-RESERVED-R7-WITHOUT-OMIT %s
+// RUN: %clang -target thumbv6m-none-eabi -### -ffixed-r7 -fno-omit-frame-pointer -c %s 2>&1 | FileCheck 

[PATCH] D57102: [extra] unit tests enable crash-recovery cases on FreeBSD

[David CARLIER via Phabricator via cfe-commits]

devnexen created this revision.
devnexen added a reviewer: steveire.
devnexen created this object with visibility "All Users".
Herald added a subscriber: cfe-commits.

Seems the previous statement does not hold up anymore.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D57102

Files:
  test/lit.cfg


Index: test/lit.cfg
===
--- test/lit.cfg
+++ test/lit.cfg
@@ -99,11 +99,9 @@
 if lit_config.useValgrind:
 config.target_triple += '-vg'
 
+config.available_features.add('crash-recovery')
 # Set available features we allow tests to conditionalize on.
 #
-# As of 2011.08, crash-recovery tests still do not pass on FreeBSD.
-if platform.system() not in ['FreeBSD']:
-config.available_features.add('crash-recovery')
 
 # Shell execution
 if execute_external:


Index: test/lit.cfg
===
--- test/lit.cfg
+++ test/lit.cfg
@@ -99,11 +99,9 @@
 if lit_config.useValgrind:
 config.target_triple += '-vg'
 
+config.available_features.add('crash-recovery')
 # Set available features we allow tests to conditionalize on.
 #
-# As of 2011.08, crash-recovery tests still do not pass on FreeBSD.
-if platform.system() not in ['FreeBSD']:
-config.available_features.add('crash-recovery')
 
 # Shell execution
 if execute_external:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

Stacks for getFile() from PCH files look like so:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00010005a1fb
clang-cl`clang::FileManager::getFile(this=0x0001106111f0,
Filename=(Data =
"/Users/thakis/src/chrome/src/out/gnwin/../../third_party/skia/include/core/SkPoint3.h",
Length = 85), openFile=false, CacheFailure=true) at FileManager.cpp:187
frame #1: 0x000103c44b2e
clang-cl`clang::ASTReader::getInputFile(this=0x00011104c200,
F=0x00011104fc00, ID=3, Complain=true) at ASTReader.cpp:2120
frame #2: 0x000103c5173b
clang-cl`clang::ASTReader::ReadControlBlock(this=0x00011104c200,
F=0x00011104fc00, Loaded=0x7fff5fbfa130,
ImportedBy=0x, ClientLoadCapabilities=0) at
ASTReader.cpp:2398
frame #3: 0x000103c5381b
clang-cl`clang::ASTReader::ReadASTCore(this=0x00011104c200,
FileName=(Data =
"obj/third_party/blink/renderer/core/layout/layout_cc.pch", Length = 56),
Type=MK_PCH, ImportLoc=(ID = 0), ImportedBy=0x,
Loaded=0x7fff5fbfa130, ExpectedSize=0, ExpectedModTime=0,
ExpectedSignature=ASTFileSignature @ 0x7fff5fbf9d58,
ClientLoadCapabilities=0) at ASTReader.cpp:4208
frame #4: 0x000103c5fe95
clang-cl`clang::ASTReader::ReadAST(this=0x00011104c200, FileName=(Data
= "obj/third_party/blink/renderer/core/layout/layout_cc.pch", Length = 56),
Type=MK_PCH, ImportLoc=(ID = 0), ClientLoadCapabilities=0,
Imported=0x) at ASTReader.cpp:3883
frame #5: 0x000100a49542
clang-cl`clang::CompilerInstance::createPCHExternalASTSource(Path=(Data =
"obj/third_party/blink/renderer/core/layout/layout_cc.pch", Length = 56),
Sysroot=(Data = "/", Length = 1), DisablePCHValidation=false,
AllowPCHWithCompilerErrors=false, PP=0x000111037e18,
Context=0x000111042200, PCHContainerRdr=0x00011060d2b0,
Extensions=ArrayRef > @
0x7fff5fbfa430, DependencyFile=0x,
DependencyCollectors=ArrayRef
> @ 0x7fff5fbfa448, DeserializationListener=0x,
OwnDeserializationListener=false, Preamble=false,
UseGlobalModuleIndex=true) at CompilerInstance.cpp:532
frame #6: 0x000100a4909b
clang-cl`clang::CompilerInstance::createPCHExternalASTSource(this=0x00011060d000,
Path=(Data = "obj/third_party/blink/renderer/core/layout/layout_cc.pch",
Length = 56), DisablePCHValidation=false, AllowPCHWithCompilerErrors=false,
DeserializationListener=0x,
OwnDeserializationListener=false) at CompilerInstance.cpp:490
frame #7: 0x000100ada301
clang-cl`clang::FrontendAction::BeginSourceFile(this=0x00011060eee0,
CI=0x00011060d000, RealInput=0x000110614a60) at
FrontendAction.cpp:859
frame #8: 0x000100a4cdf8
clang-cl`clang::CompilerInstance::ExecuteAction(this=0x00011060d000,
Act=0x00011060eee0) at CompilerInstance.cpp:953
frame #9: 0x000100b5c21f
clang-cl`clang::ExecuteCompilerInvocation(Clang=0x00011060d000) at
ExecuteCompilerInvocation.cpp:267
frame #10: 0x00011ac9 clang-cl`cc1_main(Argv=ArrayRef @ 0x7fff5fbfb4c8,
Argv0="../../../../llvm-mono-2/out/gn/bin/clang-cl",
MainAddr=0x000100028110) at cc1_main.cpp:218
frame #11: 0x00010002999f
clang-cl`ExecuteCC1Tool(argv=ArrayRef @ 0x7fff5fbfba78,
Tool=(Data = "", Length = 0)) at driver.cpp:309
frame #12: 0x000100028a81 clang-cl`main(argc_=409,
argv_=0x7fff5fbfd170) at driver.cpp:381
frame #13: 0x7fffb015c235 libdyld.dylib`start + 1

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00010005a1fb
clang-cl`clang::FileManager::getFile(this=0x0001106111f0,
Filename=(Data =
"/Users/thakis/src/chrome/src/out/gnwin/../../third_party/skia/include/core/SkPoint3.h",
Length = 85), openFile=false, CacheFailure=true) at FileManager.cpp:187
frame #1: 0x000103c476e3
clang-cl`clang::serialization::reader::HeaderFileInfoTrait::EqualKey(this=0x7fff5fbf5f80,
Key=0x7fff5fbf6068)::$_1::operator()(clang::serialization::reader::HeaderFileInfoTrait::internal_key_type
const&) const at ASTReader.cpp:1723
frame #2: 0x000103c4759f
clang-cl`clang::serialization::reader::HeaderFileInfoTrait::EqualKey(this=0x000113a16c88,
a=0x7fff5fbf6068, b=0x7fff5fbf6138) at ASTReader.cpp:1726
frame #3: 0x000103cd5c1e
clang-cl`llvm::OnDiskChainedHashTable::find_hashed(this=0x000113a16c70,
IKey=0x7fff5fbf6138, KeyHash=986574196, InfoPtr=0x000113a16c88) at
OnDiskHashTable.h:390
frame #4: 0x000103cd59b8
clang-cl`llvm::OnDiskChainedHashTable::find(this=0x000113a16c70,
EKey=0x7fff5fbf6538, InfoPtr=0x) at
OnDiskHashTable.h:345
frame #5: 0x000103cd58b9 clang-cl`(anonymous
namespace)::HeaderFileInfoVisitor::operator(this=0x7fff5fbf6538,
M=0x00011104fc00)(clang::serialization::ModuleFile&) at
ASTReader.cpp:5683
frame #6: 0x000103cd5850 clang-cl`bool llvm::function_ref::callback_fn

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added reviewers: aaron.ballman, steveire.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.

Store the controlling expression, the association expressions and the 
corresponding
`TypeSourceInfo`s as trailing objects.

Additionally use the bit-fields of `Stmt` to store one `SourceLocation`, saving 
one
additional pointer. This saves 3 pointers in total per `GenericSelectionExpr`.


Repository:
  rC Clang

https://reviews.llvm.org/D57104

Files:
  include/clang/AST/Expr.h
  include/clang/AST/Stmt.h
  lib/AST/Expr.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaPseudoObject.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp

Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -968,18 +968,21 @@
 
 void ASTStmtWriter::VisitGenericSelectionExpr(GenericSelectionExpr *E) {
   VisitExpr(E);
-  Record.push_back(E->getNumAssocs());
-
-  Record.AddStmt(E->getControllingExpr());
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
-Record.AddTypeSourceInfo(E->getAssocTypeSourceInfo(I));
-Record.AddStmt(E->getAssocExpr(I));
-  }
-  Record.push_back(E->isResultDependent() ? -1U : E->getResultIndex());
 
+  Record.push_back(E->getNumAssocs());
+  Record.push_back(E->ResultIndex);
   Record.AddSourceLocation(E->getGenericLoc());
   Record.AddSourceLocation(E->getDefaultLoc());
   Record.AddSourceLocation(E->getRParenLoc());
+
+  Stmt **Stmts = E->getTrailingObjects();
+  for (unsigned I = 0, N = E->getNumAssocs() + 1; I < N; ++I)
+Record.AddStmt(Stmts[I]);
+
+  TypeSourceInfo **TSIs = E->getTrailingObjects();
+  for (unsigned I = 0, N = E->getNumAssocs(); I < N; ++I)
+Record.AddTypeSourceInfo(TSIs[I]);
+
   Code = serialization::EXPR_GENERIC_SELECTION;
 }
 
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -1022,21 +1022,21 @@
 
 void ASTStmtReader::VisitGenericSelectionExpr(GenericSelectionExpr *E) {
   VisitExpr(E);
-  E->NumAssocs = Record.readInt();
-  E->AssocTypes = new (Record.getContext()) TypeSourceInfo*[E->NumAssocs];
-  E->SubExprs =
-   new(Record.getContext()) Stmt*[GenericSelectionExpr::END_EXPR+E->NumAssocs];
-
-  E->SubExprs[GenericSelectionExpr::CONTROLLING] = Record.readSubExpr();
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
-E->AssocTypes[I] = GetTypeSourceInfo();
-E->SubExprs[GenericSelectionExpr::END_EXPR+I] = Record.readSubExpr();
-  }
-  E->ResultIndex = Record.readInt();
 
-  E->GenericLoc = ReadSourceLocation();
+  unsigned NumAssocs = Record.readInt();
+  assert(NumAssocs == E->getNumAssocs() && "Wrong NumAssocs!");
+  E->ResultIndex = Record.readInt();
+  E->GenericSelectionExprBits.GenericLoc = ReadSourceLocation();
   E->DefaultLoc = ReadSourceLocation();
   E->RParenLoc = ReadSourceLocation();
+
+  Stmt **Stmts = E->getTrailingObjects();
+  for (unsigned I = 0, N = NumAssocs + 1; I < N; ++I)
+Stmts[I] = Record.readSubExpr();
+
+  TypeSourceInfo **TSIs = E->getTrailingObjects();
+  for (unsigned I = 0, N = NumAssocs; I < N; ++I)
+TSIs[I] = GetTypeSourceInfo();
 }
 
 void ASTStmtReader::VisitPseudoObjectExpr(PseudoObjectExpr *E) {
@@ -2675,7 +2675,9 @@
   break;
 
 case EXPR_GENERIC_SELECTION:
-  S = new (Context) GenericSelectionExpr(Empty);
+  S = GenericSelectionExpr::CreateEmpty(
+  Context,
+  /*NumAssocs=*/Record[ASTStmtReader::NumExprFields]);
   break;
 
 case EXPR_OBJC_STRING_LITERAL:
Index: lib/Sema/SemaPseudoObject.cpp
===
--- lib/Sema/SemaPseudoObject.cpp
+++ lib/Sema/SemaPseudoObject.cpp
@@ -150,15 +150,10 @@
   assocTypes[i] = gse->getAssocTypeSourceInfo(i);
 }
 
-return new (S.Context) GenericSelectionExpr(S.Context,
-gse->getGenericLoc(),
-gse->getControllingExpr(),
-assocTypes,
-assocs,
-gse->getDefaultLoc(),
-gse->getRParenLoc(),
-  gse->containsUnexpandedParameterPack(),
-resultIndex);
+return GenericSelectionExpr::Create(
+S.Context, gse->getGenericLoc(), gse->getControllingExpr(),
+assocTypes, assocs, gse->getDefaultLoc(), gse->getRParenLoc(),
+gse->containsUnexpandedParameterPack(), resultIndex);
   }
 
   if (ChooseExp

r351969 - Merge similar target diagnostics for interrupt attribute into one; NFC

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Wed Jan 23 10:02:17 2019
New Revision: 351969

URL: http://llvm.org/viewvc/llvm-project?rev=351969&view=rev
Log:
Merge similar target diagnostics for interrupt attribute into one; NFC

Patch by Kristina Bessonova!

Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaDeclAttr.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=351969&r1=351968&r2=351969&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Jan 23 10:02:17 
2019
@@ -261,22 +261,14 @@ def err_anyx86_interrupt_called : Error<
 def warn_arm_interrupt_calling_convention : Warning<
"call to function without interrupt attribute could clobber interruptee's 
VFP registers">,
InGroup;
-def warn_mips_interrupt_attribute : Warning<
-   "MIPS 'interrupt' attribute only applies to functions that have "
-   "%select{no parameters|a 'void' return type}0">,
+def warn_interrupt_attribute_invalid : Warning<
+   "%select{MIPS|MSP430|RISC-V}0 'interrupt' attribute only applies to "
+   "functions that have %select{no parameters|a 'void' return type}1">,
InGroup;
 def warn_riscv_repeated_interrupt_attribute : Warning<
   "repeated RISC-V 'interrupt' attribute">, InGroup;
 def note_riscv_repeated_interrupt_attribute : Note<
   "repeated RISC-V 'interrupt' attribute is here">;
-def warn_riscv_interrupt_attribute : Warning<
-   "RISC-V 'interrupt' attribute only applies to functions that have "
-   "%select{no parameters|a 'void' return type}0">,
-   InGroup;
-def warn_msp430_interrupt_attribute : Warning<
-   "MSP430 'interrupt' attribute only applies to functions that have "
-   "%select{no parameters|a 'void' return type}0">,
-   InGroup;
 def warn_unused_parameter : Warning<"unused parameter %0">,
   InGroup, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,

Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=351969&r1=351968&r2=351969&view=diff
==
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Jan 23 10:02:17 2019
@@ -5543,14 +5543,14 @@ static void handleMSP430InterruptAttr(Se
   }
 
   if (hasFunctionProto(D) && getFunctionOrMethodNumParams(D) != 0) {
-S.Diag(D->getLocation(), diag::warn_msp430_interrupt_attribute)
-<< 0;
+S.Diag(D->getLocation(), diag::warn_interrupt_attribute_invalid)
+<< /*MSP430*/ 1 << 0;
 return;
   }
 
   if (!getFunctionOrMethodResultType(D)->isVoidType()) {
-S.Diag(D->getLocation(), diag::warn_msp430_interrupt_attribute)
-<< 1;
+S.Diag(D->getLocation(), diag::warn_interrupt_attribute_invalid)
+<< /*MSP430*/ 1 << 1;
 return;
   }
 
@@ -5618,14 +5618,14 @@ static void handleMipsInterruptAttr(Sema
   }
 
   if (hasFunctionProto(D) && getFunctionOrMethodNumParams(D) != 0) {
-S.Diag(D->getLocation(), diag::warn_mips_interrupt_attribute)
-<< 0;
+S.Diag(D->getLocation(), diag::warn_interrupt_attribute_invalid)
+<< /*MIPS*/ 0 << 0;
 return;
   }
 
   if (!getFunctionOrMethodResultType(D)->isVoidType()) {
-S.Diag(D->getLocation(), diag::warn_mips_interrupt_attribute)
-<< 1;
+S.Diag(D->getLocation(), diag::warn_interrupt_attribute_invalid)
+<< /*MIPS*/ 0 << 1;
 return;
   }
 
@@ -5772,12 +5772,14 @@ static void handleRISCVInterruptAttr(Sem
   }
 
   if (hasFunctionProto(D) && getFunctionOrMethodNumParams(D) != 0) {
-S.Diag(D->getLocation(), diag::warn_riscv_interrupt_attribute) << 0;
+S.Diag(D->getLocation(), diag::warn_interrupt_attribute_invalid)
+  << /*RISC-V*/ 2 << 0;
 return;
   }
 
   if (!getFunctionOrMethodResultType(D)->isVoidType()) {
-S.Diag(D->getLocation(), diag::warn_riscv_interrupt_attribute) << 1;
+S.Diag(D->getLocation(), diag::warn_interrupt_attribute_invalid)
+  << /*RISC-V*/ 2 << 1;
 return;
   }
 


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

[PATCH] D57012: Merge similar target diagnostics for interrupt attribute into one. NFC

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

I've commit in r351969, thank you for the patch!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57012



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

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added reviewers: aaron.ballman, steveire.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.

Introduce a new class `GenericSelectionExpr::Association` which bundle together
an association expression and its `TypeSourceInfo`.

An iterator `GenericSelectionExpr::AssociationIterator` is additionally added 
to make
it possible to iterator over ranges of `Associations`. This iterator is a kind 
of proxy
iterator which abstract how exactly the expressions and the `TypeSourceInfo`s 
are stored.

Note: I have addressed all but 3 of the inline comments in D57098 
. The unaddressed comments
are still marked "not done" in D57098 .


Repository:
  rC Clang

https://reviews.llvm.org/D57106

Files:
  include/clang/AST/Expr.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/AST/StmtDataCollectors.td
  lib/AST/ASTDumper.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/StmtProfile.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaPseudoObject.cpp
  lib/Sema/TreeTransform.h

Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -9071,10 +9071,10 @@
 
   SmallVector AssocExprs;
   SmallVector AssocTypes;
-  for (unsigned i = 0; i != E->getNumAssocs(); ++i) {
-TypeSourceInfo *TS = E->getAssocTypeSourceInfo(i);
-if (TS) {
-  TypeSourceInfo *AssocType = getDerived().TransformType(TS);
+  for (GenericSelectionExpr::Association Assoc : E->associations()) {
+TypeSourceInfo *TSI = Assoc.getTypeSourceInfo();
+if (TSI) {
+  TypeSourceInfo *AssocType = getDerived().TransformType(TSI);
   if (!AssocType)
 return ExprError();
   AssocTypes.push_back(AssocType);
@@ -9082,7 +9082,8 @@
   AssocTypes.push_back(nullptr);
 }
 
-ExprResult AssocExpr = getDerived().TransformExpr(E->getAssocExpr(i));
+ExprResult AssocExpr =
+getDerived().TransformExpr(Assoc.getAssociationExpr());
 if (AssocExpr.isInvalid())
   return ExprError();
 AssocExprs.push_back(AssocExpr.get());
Index: lib/Sema/SemaPseudoObject.cpp
===
--- lib/Sema/SemaPseudoObject.cpp
+++ lib/Sema/SemaPseudoObject.cpp
@@ -140,19 +140,22 @@
 unsigned resultIndex = gse->getResultIndex();
 unsigned numAssocs = gse->getNumAssocs();
 
-SmallVector assocs(numAssocs);
-SmallVector assocTypes(numAssocs);
-
-for (unsigned i = 0; i != numAssocs; ++i) {
-  Expr *assoc = gse->getAssocExpr(i);
-  if (i == resultIndex) assoc = rebuild(assoc);
-  assocs[i] = assoc;
-  assocTypes[i] = gse->getAssocTypeSourceInfo(i);
+SmallVector assocExprs;
+SmallVector assocTypes;
+assocExprs.reserve(numAssocs);
+assocTypes.reserve(numAssocs);
+
+for (GenericSelectionExpr::Association assoc : gse->associations()) {
+  Expr *assocExpr = assoc.getAssociationExpr();
+  if (assoc.isSelected())
+assocExpr = rebuild(assocExpr);
+  assocExprs.push_back(assocExpr);
+  assocTypes.push_back(assoc.getTypeSourceInfo());
 }
 
 return GenericSelectionExpr::Create(
 S.Context, gse->getGenericLoc(), gse->getControllingExpr(),
-assocTypes, assocs, gse->getDefaultLoc(), gse->getRParenLoc(),
+assocTypes, assocExprs, gse->getDefaultLoc(), gse->getRParenLoc(),
 gse->containsUnexpandedParameterPack(), resultIndex);
   }
 
Index: lib/Sema/SemaExprObjC.cpp
===
--- lib/Sema/SemaExprObjC.cpp
+++ lib/Sema/SemaExprObjC.cpp
@@ -4332,14 +4332,16 @@
 assert(!gse->isResultDependent());
 
 unsigned n = gse->getNumAssocs();
-SmallVector subExprs(n);
-SmallVector subTypes(n);
-for (unsigned i = 0; i != n; ++i) {
-  subTypes[i] = gse->getAssocTypeSourceInfo(i);
-  Expr *sub = gse->getAssocExpr(i);
-  if (i == gse->getResultIndex())
+SmallVector subExprs;
+SmallVector subTypes;
+subExprs.reserve(n);
+subTypes.reserve(n);
+for (GenericSelectionExpr::Association assoc : gse->associations()) {
+  subTypes.push_back(assoc.getTypeSourceInfo());
+  Expr *sub = assoc.getAssociationExpr();
+  if (assoc.isSelected())
 sub = stripARCUnbridgedCast(sub);
-  subExprs[i] = sub;
+  subExprs.push_back(sub);
 }
 
 return GenericSelectionExpr::Create(
Index: lib/AST/StmtProfile.cpp
===
--- lib/AST/StmtProfile.cpp
+++ lib/AST/StmtProfile.cpp
@@ -1259,13 +1259,13 @@
 
 void StmtProfiler::VisitGenericSelectionExpr(const GenericSelectionExpr *S) {
   VisitExpr(S);
-  for (unsigned i = 0; i != S->getNumAssocs(); ++i) {
-QualType T = S->getAssocType(i);
+  for (GenericSe

[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked 18 inline comments as done.
riccibruno added a comment.

In D57098#1367853 , @steveire wrote:

> In D57098#1367852 , @riccibruno 
> wrote:
>
> > In D57098#1367816 , @steveire 
> > wrote:
> >
> > > Thanks for doing this!
> > >
> > > It would be easier to review (now and in the future!) if you split it 
> > > into at least 3 commits
> > >
> > > - Refactor use trailing objects without introducing the Association class
> > > - The change to use bitfields and Stmt.h
> > > - Introduce new class GenericSelectionExpr::Association
> > >
> > >   Thanks,
> > >
> > >   Stephen
> >
> >
> > Is it okay if I split it into:
> >
> > 1. `llvm::TrailingObjects` + bit-fields of `Stmt`
> > 2. Introduce `GenericSelectionExpr::Association`
> >
> >   since using `llvm::TrailingObjects` and using the bit-fields of `Stmt` 
> > are closely related and come under the title "[AST] Pack 
> > GenericSelectionExpr".
>
>
> I didn't realize that, so your proposal sounds good to me!


Split into D57104  and D57106 



Repository:
  rC Clang

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

https://reviews.llvm.org/D57098



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

[PATCH] D57001: [libunwind] Don't define unw_fpreg_t to uint64_t for __ARM_DWARF_EH__

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I have no problem with the code change, but no context on whether or not it is 
correct.
I'm hoping some other people familiar with ARM and DWARF can chime in.


Repository:
  rUNW libunwind

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

https://reviews.llvm.org/D57001



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

[PATCH] D57101: [AST] Fix addr space of result type for dereference operator

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

What?  No, the l-value should still be qualified like the pointee type.  An 
l-value-to-r-value conversion should remove the qualifier, but not just a 
dereference.


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

https://reviews.llvm.org/D57101



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

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D57087#1367610 , @alexfh wrote:

> I tend to think that a better migration strategy is to change the compiler to 
> a C++11-compatible one first, and then turn on C++11 mode and migrate the 
> code (possibly file-by-file or with a different granularity). But if you 
> observe a situation where compatibility macros for C++11 constructs are 
> actually a better way to migrate, then the proposed functionality makes sense.


@alexfh,  I couldn't agree more, however unfortunately if you are having to 
support a common code base which also supports older platforms like HPUX, 
Solaris, AIX even getting a C++11 compiler can be more of a challenge! Those 
platforms have expensive commercial compilers but are often lacking behind the 
standards.  If your lucky you can find a  gcc that will work but it tends to be 
a low version one. And then building a later gcc on those platforms is often a 
challenge too, even if you can get it and your code to build you often meet 
other hard to find issues with the final binary.

This is similar to the discussion about getting clang to compile with a minimum 
C++14 or C++17 compiler, it can be hard to pull the toolchain up to the level 
where all the supported platforms still work.

Having said all this I appreciate the code review comments, let me take a look 
at rewriting the bits you highlighted...which I totally agree with and think it 
makes for a cleaner solution. (I was trying not alter the code too much around 
line 120)


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

https://reviews.llvm.org/D57087



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

[PATCH] D56760: Add a new builtin: __builtin_dynamic_object_size

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D56760#1367915 , @jfb wrote:

> @rsmith, what do you think and how do you want to proceed? We think something 
> like what Erik implemented will catch things `_FORTIFY_SOURCE` currently 
> cannot. We agree there are valid code generation complexity concerns, yet it 
> seems like having a different spelling for the builtin helps mitigate those 
> concerns.


Yeah, I think the codegen explosion concerns are somewhat valid. It seems like 
for the most part its just a matter of keeping a value alive or doing a 
multiply or add here or there, which doesn't seem like the end of the world if 
its opt-in. The kind of pathological expressions that this is addressing seems 
like exactly the places where you would want the extra dynamic checks, like 
where you're indexing into an object with dynamically computed value with weird 
control flow or something. That being said, we could probably bail out of 
folding this in LLVM if the expression gets too complex.

So it seems like the GCC people want to keep `__builtin_object_size` static. In 
that case, I think that this current patch is the way to go. I'll post a patch 
to fix up pass_object_size too.

Thanks @jfb!


Repository:
  rC Clang

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

https://reviews.llvm.org/D56760



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

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Splitting the introduction of and porting to `Create` would significantly 
reduce the number of files touched by the 'real' change in this commit, and 
therefore reduce noise in the commit (following the idea of "do one thing per 
commit" to make the code reviewable in the future).

However, if you're opposed to that, it's not a hard requirement.




Comment at: include/clang/AST/Expr.h:5048
+// are the associated expressions.
+return 1 + getNumAssocs();
+  }

Would it be correct to use `ASSOC_EXPR_START` here instead of the magic `1`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

The way things worked before your patch is that getFile() calls for headers
that are loaded from a PCH file hit the early-ish return in

  if (UFE.isValid()) { // Already have an entry with this inode, return it.

The first stack I posted (with ReadControlBlock() in it) populates
this UniqueRealFiles cache, and then getFile() for includes in a pch when
called from the preprocessor just return "early", at least before the file
is opened. (Stack for that:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
  * frame #0: 0x00010005a2cf
clang-cl`clang::FileManager::getFile(this=0x00011200a190,
Filename=(Data =
"../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
Length = 89), openFile=true, CacheFailure=true) at FileManager.cpp:198
frame #1: 0x000102ca1cb2
clang-cl`clang::HeaderSearch::getFileAndSuggestModule(this=0x00011282e600,
FileName=(Data =
"../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
Length = 89), IncludeLoc=(ID = 4980551), Dir=0x00011071e500,
IsSystemHeaderDir=false, RequestingModule=0x,
SuggestedModule=0x7fff5fbf9020) at HeaderSearch.cpp:313
frame #2: 0x000102ca37aa
clang-cl`clang::HeaderSearch::LookupFile(this=0x00011282e600,
Filename=(Data = "third_party/skia/include/core/SkPoint3.h", Length = 40),
IncludeLoc=(ID = 4980551), isAngled=false, FromDir=0x,
CurDir=0x7fff5fbf9848, Includers=ArrayRef > @ 0x7fff5fbf7c78,
SearchPath=0x7fff5fbf9438, RelativePath=0x7fff5fbf9028,
RequestingModule=0x, SuggestedModule=0x7fff5fbf9020,
IsMapped=0x7fff5fbf9857, SkipCache=false, BuildSystemModule=false) at
HeaderSearch.cpp:756
frame #3: 0x000102d01898
clang-cl`clang::Preprocessor::LookupFile(this=0x00011282f618,
FilenameLoc=(ID = 4980551), Filename=(Data =
"third_party/skia/include/core/SkPoint3.h", Length = 40), isAngled=false,
FromDir=0x, FromFile=0x,
CurDir=0x7fff5fbf9848, SearchPath=0x7fff5fbf9438,
RelativePath=0x7fff5fbf9028, SuggestedModule=0x7fff5fbf9020,
IsMapped=0x7fff5fbf9857, SkipCache=false) at PPDirectives.cpp:740
frame #4: 0x000102d033c4
clang-cl`clang::Preprocessor::HandleIncludeDirective(this=0x00011282f618,
HashLoc=(ID = 4980542), IncludeTok=0x00011180a810,
LookupFrom=0x, LookupFromFile=0x,
isImport=false) at PPDirectives.cpp:1773
frame #5: 0x000102d05e3a
clang-cl`clang::Preprocessor::HandleDirective(this=0x00011282f618,
Result=0x00011180a810) at PPDirectives.cpp:942
frame #6: 0x000102cbff1f
clang-cl`clang::Lexer::LexTokenInternal(this=0x00011bc567a0,
Result=0x00011180a810, TokAtPhysicalStartOfLine=true) at Lexer.cpp:3931
frame #7: 0x000102cbc1b8
clang-cl`clang::Lexer::Lex(this=0x00011bc567a0,
Result=0x00011180a810) at Lexer.cpp:3152
frame #8: 0x000102d54dcb
clang-cl`clang::Preprocessor::Lex(this=0x00011282f618,
Result=0x00011180a810) at Preprocessor.cpp:868
frame #9: 0x000102f235f9
clang-cl`clang::Parser::ConsumeBrace(this=0x00011180a800) at
Parser.h:563
frame #10: 0x000102f2a21a
clang-cl`clang::BalancedDelimiterTracker::consumeClose(this=0x7fff5fbfa560)
at RAIIObjectsForParser.h:429
frame #11: 0x000102e788e5
clang-cl`clang::Parser::ParseInnerNamespace(this=0x00011180a800,
InnerNSs=0x7fff5fbfa6e0, index=0, InlineLoc=0x7fff5fbfa7a0,
attrs=0x7fff5fbfa6b8, Tracker=0x7fff5fbfa560) at
ParseDeclCXX.cpp:250
frame #12: 0x000102e77dd6
clang-cl`clang::Parser::ParseNamespace(this=0x00011180a800,
Context=FileContext, DeclEnd=0x7fff5fbfaa70, InlineLoc=(ID = 0)) at
ParseDeclCXX.cpp:223
frame #13: 0x000102e5af29
clang-cl`clang::Parser::ParseDeclaration(this=0x00011180a800,
Context=FileContext, DeclEnd=0x7fff5fbfaa70, attrs=0x7fff5fbfac40)
at ParseDecl.cpp:1714
frame #14: 0x000102f25dc3
clang-cl`clang::Parser::ParseExternalDeclaration(this=0x00011180a800,
attrs=0x7fff5fbfac40, DS=0x) at Parser.cpp:788
frame #15: 0x000102f24f06
clang-cl`clang::Parser::ParseTopLevelDecl(this=0x00011180a800,
Result=0x7fff5fbfad78) at Parser.cpp:609
frame #16: 0x000102e4a076
clang-cl`clang::ParseAST(S=0x0001118fb600, PrintStats=true,
SkipFunctionBodies=false) at ParseAST.cpp:156
frame #17: 0x000100add312
clang-cl`clang::ASTFrontendAction::ExecuteAction(this=0x000112007eb0)
at FrontendAction.cpp:1035
frame #18: 0x0001004f6cbd
clang-cl`clang::CodeGenAction::ExecuteAction(this=0x000112007eb0) at
CodeGenAction.cpp:1047
frame #19: 0x000100adc920
clang-cl`clang::FrontendAction::Execute(this=0x000112007eb0) at
FrontendAction.cpp:934
frame #20: 0x00

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

I haven't done a full review as it's not obvious what parts of the diff relate 
to which separate change. Perhaps Aaron will review and approve though for you 
anyway.

Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Just in case you didn't know, you could have (and still can!) just update 
https://reviews.llvm.org/D57098 instead of creating a new PR.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57106



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

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.



Comment at: lib/AST/ASTDumper.cpp:1465
 
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
+  for (GenericSelectionExpr::ConstAssociation Assoc : E->associations()) {
 dumpChild([=] {

Range for loops in this file generally use `auto`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57106



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

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked an inline comment as done.
riccibruno added a comment.

In D57104#1368055 , @steveire wrote:

> Splitting the introduction of and porting to `Create` would significantly 
> reduce the number of files touched by the 'real' change in this commit, and 
> therefore reduce noise in the commit (following the idea of "do one thing per 
> commit" to make the code reviewable in the future).
>
> However, if you're opposed to that, it's not a hard requirement.


To be honest I don't really see the point. This is just the usual pattern of 
having a `Create` function doing the allocation,
which then dispatch to the private constructor with a placement new for the 
initialization.




Comment at: include/clang/AST/Expr.h:5048
+// are the associated expressions.
+return 1 + getNumAssocs();
+  }

steveire wrote:
> Would it be correct to use `ASSOC_EXPR_START` here instead of the magic `1`?
Eh maybe ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 183131.
JonasToth added a comment.

- [Fix] make class name correct


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100

Files:
  clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tidy/bugprone/ExceptionEscapeCheck.h
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tidy/utils/ExceptionAnalyzer.h
  docs/clang-tidy/checks/bugprone-exception-escape.rst
  test/clang-tidy/bugprone-exception-escape.cpp

Index: test/clang-tidy/bugprone-exception-escape.cpp
===
--- test/clang-tidy/bugprone-exception-escape.cpp
+++ test/clang-tidy/bugprone-exception-escape.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- -extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'}]}" --
+// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- -extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1;ignored2'}, {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1;enabled2;enabled3'}]}" --
 
 struct throwing_destructor {
   ~throwing_destructor() {
Index: docs/clang-tidy/checks/bugprone-exception-escape.rst
===
--- docs/clang-tidy/checks/bugprone-exception-escape.rst
+++ docs/clang-tidy/checks/bugprone-exception-escape.rst
@@ -28,12 +28,12 @@
 
 .. option:: FunctionsThatShouldNotThrow
 
-   Comma separated list containing function names which should not throw. An
+   Semicolon separated list containing function names which should not throw. An
example value for this parameter can be ``WinMain`` which adds function
``WinMain()`` in the Windows API to the list of the funcions which should
not throw. Default value is an empty string.
 
 .. option:: IgnoredExceptions
 
-   Comma separated list containing type names which are not counted as thrown
+   Semicolon separated list containing type names which are not counted as thrown
exceptions in the check. Default value is an empty string.
Index: clang-tidy/utils/ExceptionAnalyzer.h
===
--- /dev/null
+++ clang-tidy/utils/ExceptionAnalyzer.h
@@ -0,0 +1,50 @@
+//===--- ExceptionAnalyzer.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
+
+#include "clang/AST/ASTContext.h"
+#include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/StringSet.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+/// This class analysis if a `FunctionDecl` can in principle throw an exception,
+/// either directly or indirectly.
+/// It can be configured to ignore some exception types, especially
+/// `std::bad_alloc` can be disabled as using dynamic memory will always
+/// give the possibility of an exception.
+class ExceptionAnalyzer {
+public:
+  ExceptionAnalyzer() = default;
+
+  bool throwsException(const FunctionDecl *Func);
+  void ignoreExceptions(llvm::StringSet<> ExceptionNames) {
+IgnoredExceptions = std::move(ExceptionNames);
+  }
+
+private:
+  using TypeVec = llvm::SmallVector;
+
+  TypeVec throwsException(const FunctionDecl *Func,
+  llvm::SmallSet &CallStack);
+  TypeVec throwsException(const Stmt *St, const TypeVec &Caught,
+  llvm::SmallSet &CallStack);
+  bool isIgnoredExceptionType(const Type *Exception);
+
+  llvm::StringSet<> IgnoredExceptions;
+  llvm::DenseMap FunctionCache;
+};
+} // namespace utils
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
Index: clang-tidy/utils/ExceptionAnalyzer.cpp
===
--- /dev/null
+++ clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -0,0 +1,146 @@
+#include "ExceptionAnalyzer.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+static bool isBaseOf(const Type *DerivedType, const Type *BaseType) {
+  const auto *DerivedClass = DerivedType->getAsCXXRecordDecl();
+  const auto *BaseClass = BaseType->getAsCXXRecordDecl();
+  if (!DerivedClass || !BaseClass)
+return false;
+
+  return !DerivedClass->forallBa

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:35
+  std::vector FunctionsThatShouldNotThrowVec =
+  utils::options::parseStringList(RawFunctionsThatShouldNotThrow);
   FunctionsThatShouldNotThrow.insert(FunctionsThatShouldNotThrowVec.begin(),

Does it also accept ',' delimitter?
If not, i'm not sure how good of an idea it is to break existing configs..



Comment at: clang-tidy/utils/ExceptionAnalyzer.cpp:1
+#include "ExceptionAnalyzer.h"
+

Standard licence blurb missing.



Comment at: test/clang-tidy/bugprone-exception-escape.cpp:1
-// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- 
-extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: 
bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, {key: 
bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 
'enabled1,enabled2,enabled3'}]}" --
+// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- 
-extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: 
bugprone-exception-escape.IgnoredExceptions, value: 'ignored1;ignored2'}, {key: 
bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 
'enabled1;enabled2;enabled3'}]}" --
 

Hm, this may be a breaking change, and the check existed in 7.0 already.



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100



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

[PATCH] D57108: [clang-tidy] diagnose possibiltiy to add 'noexcept' in modernize-use-noexcept

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: aaron.ballman, alexfh, hokein.
Herald added subscribers: xazax.hun, mgorny.

This patch utilizes the refactored `ExceptionAnalyzer` for
`modernize-use-noexcept` to diagnose possibilties to introduce `noexcept`
if there is no exception specification at all.
A follow-up patch will then implement the code-transformation if this patch
is useful.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D57108

Files:
  clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tidy/bugprone/ExceptionEscapeCheck.h
  clang-tidy/modernize/UseNoexceptCheck.cpp
  clang-tidy/modernize/UseNoexceptCheck.h
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tidy/utils/ExceptionAnalyzer.h
  docs/clang-tidy/checks/bugprone-exception-escape.rst
  test/clang-tidy/bugprone-exception-escape.cpp
  test/clang-tidy/modernize-use-noexcept-new-noexcept.cpp
  test/clang-tidy/modernize-use-noexcept-opt.cpp
  test/clang-tidy/modernize-use-noexcept.cpp

Index: test/clang-tidy/modernize-use-noexcept.cpp
===
--- test/clang-tidy/modernize-use-noexcept.cpp
+++ test/clang-tidy/modernize-use-noexcept.cpp
@@ -10,9 +10,9 @@
 
 template 
 void foo() throw();
-void footest() { foo(); foo(); }
-// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 // CHECK-FIXES: void foo() noexcept;
+void footest() { foo(); foo(); }
 
 void bar() throw(...);
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw(...)' is deprecated; consider using 'noexcept(false)' instead [modernize-use-noexcept]
@@ -70,21 +70,21 @@
 
 struct S {
   void f() throw();
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 };
 void f(void (S::*)() throw());
-// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
-// CHECK-MESSAGES: :[[@LINE-2]]:22: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 // CHECK-FIXES: void f() noexcept;
 // CHECK-FIXES: void f(void (S::*)() noexcept);
 
 template 
 struct ST {
   void foo() throw();
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 };
 template 
 void ft(void (ST::*)() throw());
-// CHECK-MESSAGES: :[[@LINE-4]]:14: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
-// CHECK-MESSAGES: :[[@LINE-2]]:27: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 // CHECK-FIXES: void foo() noexcept;
 // CHECK-FIXES: void ft(void (ST::*)() noexcept);
 
Index: test/clang-tidy/modernize-use-noexcept-opt.cpp
===
--- test/clang-tidy/modernize-use-noexcept-opt.cpp
+++ test/clang-tidy/modernize-use-noexcept-opt.cpp
@@ -65,11 +65,11 @@
 
 struct S {
   void f() throw();
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-FIXES: void f() noexcept;
 };
 void f(void (S::*)() throw());
-// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
-// CHECK-MESSAGES: :[[@LINE-2]]:22: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
-// CHECK-FIXES: void f() noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 // CHECK-FIXES: void f(void (S::*)() noexcept);
 
 typedef void (*fp)(void (*fp2)(int) throw());
Index: test/clang-tidy/modernize-use-noexcept-new-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-noexcept-new-noexcept.cpp
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy %s modernize-use-noexcept %t -- 

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 3 inline comments as done.
JonasToth added inline comments.



Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:35
+  std::vector FunctionsThatShouldNotThrowVec =
+  utils::options::parseStringList(RawFunctionsThatShouldNotThrow);
   FunctionsThatShouldNotThrow.insert(FunctionsThatShouldNotThrowVec.begin(),

lebedev.ri wrote:
> Does it also accept ',' delimitter?
> If not, i'm not sure how good of an idea it is to break existing configs..
No it doesn't. It is a breaking change but at the same time it is inconsistent 
and duplicated functionality.
Should be mentioned in release notes, that for sure.



Comment at: test/clang-tidy/bugprone-exception-escape.cpp:1
-// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- 
-extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: 
bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, {key: 
bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 
'enabled1,enabled2,enabled3'}]}" --
+// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- 
-extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: 
bugprone-exception-escape.IgnoredExceptions, value: 'ignored1;ignored2'}, {key: 
bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 
'enabled1;enabled2;enabled3'}]}" --
 

lebedev.ri wrote:
> Hm, this may be a breaking change, and the check existed in 7.0 already.
> 
See above.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100



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

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 183134.
JonasToth marked an inline comment as done.
JonasToth added a comment.

- add license to exceptionanalyzer


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100

Files:
  clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tidy/bugprone/ExceptionEscapeCheck.h
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tidy/utils/ExceptionAnalyzer.h
  docs/clang-tidy/checks/bugprone-exception-escape.rst
  test/clang-tidy/bugprone-exception-escape.cpp

Index: test/clang-tidy/bugprone-exception-escape.cpp
===
--- test/clang-tidy/bugprone-exception-escape.cpp
+++ test/clang-tidy/bugprone-exception-escape.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- -extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'}]}" --
+// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- -extra-arg=-std=c++11 -extra-arg=-fexceptions -config="{CheckOptions: [{key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1;ignored2'}, {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1;enabled2;enabled3'}]}" --
 
 struct throwing_destructor {
   ~throwing_destructor() {
Index: docs/clang-tidy/checks/bugprone-exception-escape.rst
===
--- docs/clang-tidy/checks/bugprone-exception-escape.rst
+++ docs/clang-tidy/checks/bugprone-exception-escape.rst
@@ -28,12 +28,12 @@
 
 .. option:: FunctionsThatShouldNotThrow
 
-   Comma separated list containing function names which should not throw. An
+   Semicolon separated list containing function names which should not throw. An
example value for this parameter can be ``WinMain`` which adds function
``WinMain()`` in the Windows API to the list of the funcions which should
not throw. Default value is an empty string.
 
 .. option:: IgnoredExceptions
 
-   Comma separated list containing type names which are not counted as thrown
+   Semicolon separated list containing type names which are not counted as thrown
exceptions in the check. Default value is an empty string.
Index: clang-tidy/utils/ExceptionAnalyzer.h
===
--- /dev/null
+++ clang-tidy/utils/ExceptionAnalyzer.h
@@ -0,0 +1,50 @@
+//===--- ExceptionAnalyzer.h - clang-tidy ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
+
+#include "clang/AST/ASTContext.h"
+#include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/StringSet.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+/// This class analysis if a `FunctionDecl` can in principle throw an exception,
+/// either directly or indirectly.
+/// It can be configured to ignore some exception types, especially
+/// `std::bad_alloc` can be disabled as using dynamic memory will always
+/// give the possibility of an exception.
+class ExceptionAnalyzer {
+public:
+  ExceptionAnalyzer() = default;
+
+  bool throwsException(const FunctionDecl *Func);
+  void ignoreExceptions(llvm::StringSet<> ExceptionNames) {
+IgnoredExceptions = std::move(ExceptionNames);
+  }
+
+private:
+  using TypeVec = llvm::SmallVector;
+
+  TypeVec throwsException(const FunctionDecl *Func,
+  llvm::SmallSet &CallStack);
+  TypeVec throwsException(const Stmt *St, const TypeVec &Caught,
+  llvm::SmallSet &CallStack);
+  bool isIgnoredExceptionType(const Type *Exception);
+
+  llvm::StringSet<> IgnoredExceptions;
+  llvm::DenseMap FunctionCache;
+};
+} // namespace utils
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_EXCEPTION_ANALYZER_H
Index: clang-tidy/utils/ExceptionAnalyzer.cpp
===
--- clang-tidy/utils/ExceptionAnalyzer.cpp
+++ clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -1,4 +1,4 @@
-//===--- ExceptionEscapeCheck.cpp - clang-tidy-===//
+//===--- ExceptionAnalyzer.cpp - clang-tidy ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,22 +6,12 @@
 //
 //===

[PATCH] D57108: [clang-tidy] diagnose possibiltiy to add 'noexcept' in modernize-use-noexcept

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 183136.
JonasToth added a comment.

- rebase to current refactoring


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57108

Files:
  clang-tidy/bugprone/ExceptionEscapeCheck.cpp
  clang-tidy/bugprone/ExceptionEscapeCheck.h
  clang-tidy/modernize/UseNoexceptCheck.cpp
  clang-tidy/modernize/UseNoexceptCheck.h
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/ExceptionAnalyzer.cpp
  clang-tidy/utils/ExceptionAnalyzer.h
  docs/clang-tidy/checks/bugprone-exception-escape.rst
  test/clang-tidy/bugprone-exception-escape.cpp
  test/clang-tidy/modernize-use-noexcept-new-noexcept.cpp
  test/clang-tidy/modernize-use-noexcept-opt.cpp
  test/clang-tidy/modernize-use-noexcept.cpp

Index: test/clang-tidy/modernize-use-noexcept.cpp
===
--- test/clang-tidy/modernize-use-noexcept.cpp
+++ test/clang-tidy/modernize-use-noexcept.cpp
@@ -10,9 +10,9 @@
 
 template 
 void foo() throw();
-void footest() { foo(); foo(); }
-// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 // CHECK-FIXES: void foo() noexcept;
+void footest() { foo(); foo(); }
 
 void bar() throw(...);
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw(...)' is deprecated; consider using 'noexcept(false)' instead [modernize-use-noexcept]
@@ -70,21 +70,21 @@
 
 struct S {
   void f() throw();
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 };
 void f(void (S::*)() throw());
-// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
-// CHECK-MESSAGES: :[[@LINE-2]]:22: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 // CHECK-FIXES: void f() noexcept;
 // CHECK-FIXES: void f(void (S::*)() noexcept);
 
 template 
 struct ST {
   void foo() throw();
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 };
 template 
 void ft(void (ST::*)() throw());
-// CHECK-MESSAGES: :[[@LINE-4]]:14: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
-// CHECK-MESSAGES: :[[@LINE-2]]:27: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 // CHECK-FIXES: void foo() noexcept;
 // CHECK-FIXES: void ft(void (ST::*)() noexcept);
 
Index: test/clang-tidy/modernize-use-noexcept-opt.cpp
===
--- test/clang-tidy/modernize-use-noexcept-opt.cpp
+++ test/clang-tidy/modernize-use-noexcept-opt.cpp
@@ -65,11 +65,11 @@
 
 struct S {
   void f() throw();
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
+// CHECK-FIXES: void f() noexcept;
 };
 void f(void (S::*)() throw());
-// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
-// CHECK-MESSAGES: :[[@LINE-2]]:22: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
-// CHECK-FIXES: void f() noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: dynamic exception specification 'throw()' is deprecated; consider using 'noexcept' instead [modernize-use-noexcept]
 // CHECK-FIXES: void f(void (S::*)() noexcept);
 
 typedef void (*fp)(void (*fp2)(int) throw());
Index: test/clang-tidy/modernize-use-noexcept-new-noexcept.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-noexcept-new-noexcept.cpp
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy %s modernize-use-noexcept %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-noexcept.AddMissingNoexcept, value: 1}]}" \
+// RUN:   -- -std=c++11 -fexceptions
+
+void undefined();
+void undefinedNoexcept() noexcept;
+
+void empty() {}
+// CHECK-MESS

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

Here's what I think is a repro:

Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo2.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
#include "foo.h"
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
#include "foo2.h"
#include "foo.h"

#include "foo2.h"
#include "foo.h"

Then just build pch.cc with a pch file, and then use it (you can do this on
any OS; you could also use the gcc style driver if you prefer it).

The following output is with your change reverted, I added

fprintf(stderr, "stat %s %d\n", Filename.str().c_str(), openFile);

right before the call to getStatValue() in getFile, and

if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
  UFE.File = std::move(F);

at the very bottom of that function, and changed getBufferForFile() to have

if (ShouldCloseOpenFile) {
fprintf(stderr, "closing %s\n", Entry->getName().str().c_str());
  Entry->closeFile();
}


The output is:

Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
/FIpch.h pch.cc
stat pch.cc 1
opened pch.cc
closing pch.cc
stat ./pch.h 1
opened ./pch.h
closing ./pch.h
stat ./foo.h 1
opened ./foo.h
closing ./foo.h
stat pch.cc 1
opened pch.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing /Users/thakis/src/llvm-mono-2/pch.cc
stat ./pch.h 1
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
/FIpch.h client.cc
stat client.cc 1
opened client.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing client.cc
stat ./pch.h 1
stat ./foo2.h 1
opened ./foo2.h
closing ./foo2.h
stat ./foo.h 1


Note how foo.h at the end is stat'd with openFile = 1, but we don't keep
that file handle around.

Now with your patch applied, where put the

if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
UFE.File = std::move(F);

print in the block you moved up.

Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
/FIpch.h pch.cc
stat pch.cc 1
opened pch.cc
closing pch.cc
stat ./pch.h 1
opened ./pch.h
closing ./pch.h
stat ./foo.h 1
opened ./foo.h
closing ./foo.h
stat pch.cc 1
opened pch.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing /Users/thakis/src/llvm-mono-2/pch.cc
stat ./pch.h 1
opened ./pch.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
/FIpch.h client.cc
stat client.cc 1
opened client.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing client.cc
stat ./pch.h 1
opened ./pch.h
stat ./foo2.h 1
opened ./foo2.h
closing ./foo2.h
stat ./foo.h 1
opened ./foo.h
closing ./foo.h


Notice how pch.pch gets opened twice but never closed in the version with
your patch. I think this is enough to show that your patch is introducing
assumptions made by clang's PCH code. Even though the issue isn't
understood in detail, this is imho enough to revert and fix and debug
async, unless you're able to fix very quickly.



On Wed, Jan 23, 2019 at 1:23 PM Nico Weber  wrote:

> The way things worked before your patch is that getFile() calls for
> headers that are loaded from a PCH file hit the early-ish return in
>
>   if (UFE.isValid()) { // Already have an entry with this inode, return it.
>
> The first stack I posted (with ReadControlBlock() in it) populates
> this UniqueRealFiles cache, and then getFile() for includes in a pch when
> called from the preprocessor just return "early", at least before the file
> is opened. (Stack for that:
>
> (lldb) bt
> * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
>   * frame #0: 0x00010005a2cf
> clang-cl`clang::FileManager::getFile(this=0x00011200a190,
> Filename=(Data =
> "../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
> Length = 89), openFile=true, CacheFailure=true) at FileManager.cpp:198
> frame #1: 0x000102ca1cb2
> clang-cl`clang::HeaderSearch::getFileAndSuggestModule(this=0x00011282e600,
> FileName=(Data =
> "../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
> Length = 89), IncludeLoc=(ID = 4980551), Dir=0x00011071e500,
> IsSystemHeaderDir=false, RequestingModule=0x,
> SuggestedModule=0x7fff5fbf9020) at HeaderSearch.cpp:313
> frame #2: 0x000102ca37aa
> clang-cl`clang::HeaderSearch::LookupFile(this=0x00011282e600,
> Filename=(

[PATCH] D56924: Handle ObjCCategoryDecl class extensions for print

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

In D56924#1367450 , @sammccall wrote:

> This is definitely an improvement, though I don't know if it's *right*. 
> @akyrtzi, thoughts?


Yeah, I'm not sure what the desired behavior is. When writing up the test I 
noticed there is a `Obj::property`, presumably for the auto-generated getter 
method (which I then filtered out via `objcPropertyDecl`).

Where is the qualified name used?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56924



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

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/Expr.h:5043
+  //   association expressions.
+  enum { CONTROLLING = 0, ASSOC_EXPR_START = 1 };
+

Do we want to use `ControllingIndex` and `AssocExprStartIndex` and combine with 
the enum you introduced above?



Comment at: include/clang/AST/Expr.h:5048
+// are the associated expressions.
+return 1 + getNumAssocs();
+  }

riccibruno wrote:
> steveire wrote:
> > Would it be correct to use `ASSOC_EXPR_START` here instead of the magic `1`?
> Eh maybe ?
I think using this named constant adds confusion rather than clarifies things 
because it's not clear what the index has to do with the count. I think the 
comments suffice here rather than introducing a named constant with a 
sufficiently generic name.



Comment at: lib/AST/Expr.cpp:3814
+   /*isInstantiationDependent=*/true, ContainsUnexpandedParameterPack),
+  NumAssocs(AssocExprs.size()), ResultIndex(-1U), DefaultLoc(DefaultLoc),
+  RParenLoc(RParenLoc) {

`-1U` -> `ResultDependentIndex`



Comment at: lib/Serialization/ASTReaderStmt.cpp:1034
+  Stmt **Stmts = E->getTrailingObjects();
+  for (unsigned I = 0, N = NumAssocs + 1; I < N; ++I)
+Stmts[I] = Record.readSubExpr();

You should add a comment above the loop that explains the `+ 1` is for the 
controlling expression and that's why it's not needed for the type source 
information.



Comment at: lib/Serialization/ASTWriterStmt.cpp:979
+  Stmt **Stmts = E->getTrailingObjects();
+  for (unsigned I = 0, N = E->getNumAssocs() + 1; I < N; ++I)
+Record.AddStmt(Stmts[I]);

Similar comment here as above.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

On Wed, Jan 23, 2019 at 2:17 PM Nico Weber  wrote:

> Here's what I think is a repro:
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo2.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
> #include "foo.h"
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
> #include "foo2.h"
> #include "foo.h"
>
> #include "foo2.h"
> #include "foo.h"
>
> Then just build pch.cc with a pch file, and then use it (you can do this
> on any OS; you could also use the gcc style driver if you prefer it).
>
> The following output is with your change reverted, I added
>
> fprintf(stderr, "stat %s %d\n", Filename.str().c_str(), openFile);
>
> right before the call to getStatValue() in getFile, and
>
> if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
>   UFE.File = std::move(F);
>
> at the very bottom of that function, and changed getBufferForFile() to have
>
> if (ShouldCloseOpenFile) {
> fprintf(stderr, "closing %s\n", Entry->getName().str().c_str());
>   Entry->closeFile();
> }
>
>
> The output is:
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
> /FIpch.h pch.cc
> stat pch.cc 1
> opened pch.cc
> closing pch.cc
> stat ./pch.h 1
> opened ./pch.h
> closing ./pch.h
> stat ./foo.h 1
> opened ./foo.h
> closing ./foo.h
> stat pch.cc 1
> opened pch.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing /Users/thakis/src/llvm-mono-2/pch.cc
> stat ./pch.h 1
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
> /FIpch.h client.cc
> stat client.cc 1
> opened client.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing client.cc
> stat ./pch.h 1
> stat ./foo2.h 1
> opened ./foo2.h
> closing ./foo2.h
> stat ./foo.h 1
>
>
> Note how foo.h at the end is stat'd with openFile = 1, but we don't keep
> that file handle around.
>
> Now with your patch applied, where put the
>
> if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
> UFE.File = std::move(F);
>
> print in the block you moved up.
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
> /FIpch.h pch.cc
> stat pch.cc 1
> opened pch.cc
> closing pch.cc
> stat ./pch.h 1
> opened ./pch.h
> closing ./pch.h
> stat ./foo.h 1
> opened ./foo.h
> closing ./foo.h
> stat pch.cc 1
> opened pch.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing /Users/thakis/src/llvm-mono-2/pch.cc
> stat ./pch.h 1
> opened ./pch.h
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
> /FIpch.h client.cc
> stat client.cc 1
> opened client.cc
> stat pch.pch 1
> opened pch.pch
> closing pch.pch
> stat /Users/thakis/src/llvm-mono-2/foo.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.h 0
> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
> closing client.cc
> stat ./pch.h 1
> opened ./pch.h
> stat ./foo2.h 1
> opened ./foo2.h
> closing ./foo2.h
> stat ./foo.h 1
> opened ./foo.h
> closing ./foo.h
>
>
> Notice how pch.pch gets opened twice but never closed in the version with
> your patch.
>

Sorry, I mean pch.h. But that's probably just from the /FI flag. While this
is an example of a file that's opened and not closed with your patch, it'd
be nice to have a repro that shows this from an #include line, not from an
/FI flag. Let me look at this a bit more. Sorry for the noise.


> I think this is enough to show that your patch is introducing assumptions
> made by clang's PCH code. Even though the issue isn't understood in detail,
> this is imho enough to revert and fix and debug async, unless you're able
> to fix very quickly.
>
>
>
> On Wed, Jan 23, 2019 at 1:23 PM Nico Weber  wrote:
>
>> The way things worked before your patch is that getFile() calls for
>> headers that are loaded from a PCH file hit the early-ish return in
>>
>>   if (UFE.isValid()) { // Already have an entry with this inode, return
>> it.
>>
>> The first stack I posted (with ReadControlBlock() in it) populates
>> this UniqueRealFiles cache, and then getFile() for includes in a pch when
>> called from the preprocessor just return "early", at least before the file
>> is opened. (Stack for that:
>>
>> (lldb) bt
>> * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
>>   * frame #0: 0x00010005a2cf
>> clang-cl`clang::FileManager::getFile(this=0x00011200a190,
>> Filename=(Data =
>> "../../third_party/blink/renderer/core/layout/api/third_party/skia/include/core/SkPoint3.h/paint/third_party/skia/include/core/SkPoint3.hܲ?\x01",
>> Length = 89), openFile=true, CacheFailure=

[PATCH] D56935: [NewPM] Add support for new-PM plugins to clang

[Marco Elver via Phabricator via cfe-commits]

melver updated this revision to Diff 183140.
melver added a comment.

- Revert use of SmallVector


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

https://reviews.llvm.org/D56935

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1322,6 +1322,8 @@
 
   Opts.DefaultFunctionAttrs = Args.getAllArgValues(OPT_default_function_attr);
 
+  Opts.PassPlugins = Args.getAllArgValues(OPT_fpass_plugin_EQ);
+
   return Success;
 }
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5066,6 +5066,13 @@
 A->claim();
   }
 
+  // Forward -fpass-plugin=name.so to -cc1.
+  for (const Arg *A : Args.filtered(options::OPT_fpass_plugin_EQ)) {
+CmdArgs.push_back(
+Args.MakeArgString(Twine("-fpass-plugin=") + A->getValue()));
+A->claim();
+  }
+
   // Setup statistics file output.
   SmallString<128> StatsFile = getStatsFileName(Args, Output, Input, D);
   if (!StatsFile.empty())
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -37,6 +37,7 @@
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Passes/PassBuilder.h"
+#include "llvm/Passes/PassPlugin.h"
 #include "llvm/Support/BuryPointer.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -962,6 +963,16 @@
 
   PassBuilder PB(TM.get(), PGOOpt);
 
+  // Attempt to load pass plugins and register their callbacks with PB.
+  for (auto &PluginFN : CodeGenOpts.PassPlugins) {
+if (auto PassPlugin = PassPlugin::Load(PluginFN)) {
+  PassPlugin->registerPassBuilderCallbacks(PB);
+} else {
+  errs() << "Failed to load passes from '" << PluginFN
+ << "'. Request ignored.\n";
+}
+  }
+
   LoopAnalysisManager LAM(CodeGenOpts.DebugPassManager);
   FunctionAnalysisManager FAM(CodeGenOpts.DebugPassManager);
   CGSCCAnalysisManager CGAM(CodeGenOpts.DebugPassManager);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1613,6 +1613,9 @@
 def fno_rwpi : Flag<["-"], "fno-rwpi">, Group;
 def fplugin_EQ : Joined<["-"], "fplugin=">, Group, 
Flags<[DriverOption]>, MetaVarName<"">,
   HelpText<"Load the named plugin (dynamic shared object)">;
+def fpass_plugin_EQ : Joined<["-"], "fpass-plugin=">,
+  Group, Flags<[CC1Option]>, MetaVarName<"">,
+  HelpText<"Load pass plugin from a dynamic shared object file (only with new 
pass manager).">;
 def fpreserve_as_comments : Flag<["-"], "fpreserve-as-comments">, 
Group;
 def fno_preserve_as_comments : Flag<["-"], "fno-preserve-as-comments">, 
Group, Flags<[CC1Option]>,
   HelpText<"Do not preserve comments in inline assembly">;
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -288,6 +288,9 @@
 
   std::vector DefaultFunctionAttrs;
 
+  /// List of dynamic shared object files to be loaded as pass plugins.
+  std::vector PassPlugins;
+
 public:
   // Define accessors/mutators for code generation options of enumeration type.
 #define CODEGENOPT(Name, Bits, Default)


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1322,6 +1322,8 @@
 
   Opts.DefaultFunctionAttrs = Args.getAllArgValues(OPT_default_function_attr);
 
+  Opts.PassPlugins = Args.getAllArgValues(OPT_fpass_plugin_EQ);
+
   return Success;
 }
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5066,6 +5066,13 @@
 A->claim();
   }
 
+  // Forward -fpass-plugin=name.so to -cc1.
+  for (const Arg *A : Args.filtered(options::OPT_fpass_plugin_EQ)) {
+CmdArgs.push_back(
+Args.MakeArgString(Twine("-fpass-plugin=") + A->getValue()));
+A->claim();
+  }
+
   // Setup statistics file output.
   SmallString<128> StatsFile = getStatsFileName(Args, Output, Input, D);
   if (!StatsFile.empty())
Index: clang/lib/CodeGen/BackendUtil.cpp
==

[PATCH] D56935: [NewPM] Add support for new-PM plugins to clang

[Marco Elver via Phabricator via cfe-commits]

melver added a comment.

Thanks!

In D56935#1367545 , @philip.pfaffe 
wrote:

> I'm not sure what the current state of plugins on windows is. They were 
> broken and disabled last time I worked on this, but that might've changed in 
> the meantime! Worth checking.


Right, though I feel this is related to DynamicLibrary, and orthogonal to this 
patch. I don't have access to a Windows machine right now, but I can try to 
check if you feel it's urgent to land this. What do you think?


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

https://reviews.llvm.org/D56935



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

[PATCH] D53329: Generate DIFile with main program if source is not available

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Thanks for filing the bug/reducing a test case - this change should include an 
automated test case that captures this issue (check for other tests that pass 
-gembed-source to see how this might be tested, possibly expanding the existing 
test case case or introducing a new one (test/CodeGen/debug-info-embed-source.c 
looks like the place to start)

I reduced your test case down a bit further:

test.h:

  int g;

test.c:

  #include "test.h"
  #define MACRO int x;
  MACRO

Though the #include is needed to reproduce the /crash/, but even without the 
#include you can still reproduce the underlying bug - a DIFile witohut source 
and a DIFile with source in the same IR file (one compiled with -gembed-source).

Actually - is that a required invariant, that all DIFiles have source if any of 
them do? That could be a problem for LTO situations that might merge modules 
(one of which may have embedded source and another that doesn't)

Is this fix correct even when the problem occurs in a header file, for instance?

Yeah, seems this problem doesn't occur if you move everything into the header - 
I think it'd be worthwhile to understand/explain why that works out (despite 
the file ID for the macro would be associated with the source range in both 
macro-used-in-header and macro-used-in-primary-source-file cases, so why do 
they need different handling later? Maybe there's a better way to handle this 
more consistently?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D53329



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

[PATCH] D57111: Make Clang not crash on calls to destructors on incomplete pointer types

[Roman Zhikharevich via Phabricator via cfe-commits]

rzhikharevich created this revision.
rzhikharevich added a project: clang.
Herald added a subscriber: cfe-commits.

This patch fixes handling of code like this:

  c++
  struct Foo;
  
  void foo(Foo *p) {
p.~Foo();
  }


Repository:
  rC Clang

https://reviews.llvm.org/D57111

Files:
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/SemaCXX/incomplete-call.cpp


Index: clang/test/SemaCXX/incomplete-call.cpp
===
--- clang/test/SemaCXX/incomplete-call.cpp
+++ clang/test/SemaCXX/incomplete-call.cpp
@@ -48,6 +48,10 @@
   c(); // expected-error{{incomplete type in call to object of type}}
 }
 
+void test_incomplete_object_dtor(C *p) {
+  p.~C(); // expected-error{{member reference type 'C *' is a pointer; did you 
mean to use '->'?}}
+}
+
 namespace pr18542 {
   struct X {
 int count;
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -6854,8 +6854,9 @@
QualType DestructedType) {
   // If this is a record type, check if its destructor is callable.
   if (auto *RD = DestructedType->getAsCXXRecordDecl()) {
-if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
-  return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
+if (RD->hasDefinition())
+  if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
+return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
 return false;
   }
 


Index: clang/test/SemaCXX/incomplete-call.cpp
===
--- clang/test/SemaCXX/incomplete-call.cpp
+++ clang/test/SemaCXX/incomplete-call.cpp
@@ -48,6 +48,10 @@
   c(); // expected-error{{incomplete type in call to object of type}}
 }
 
+void test_incomplete_object_dtor(C *p) {
+  p.~C(); // expected-error{{member reference type 'C *' is a pointer; did you mean to use '->'?}}
+}
+
 namespace pr18542 {
   struct X {
 int count;
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -6854,8 +6854,9 @@
QualType DestructedType) {
   // If this is a record type, check if its destructor is callable.
   if (auto *RD = DestructedType->getAsCXXRecordDecl()) {
-if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
-  return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
+if (RD->hasDefinition())
+  if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
+return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
 return false;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

This might finally be a repro!

Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
foo.h foo2.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ type foo.h
-bash: type: foo.h: not found
Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
foo.h foo2.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo.h
#include "foo2.h"
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo2.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
#include "foo.h"
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
#include "bar/foo.h"
#include "bar/foo2.h"

#include "foo3.h"
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
/FIpch.h pch.cc  /Ibar
stat pch.cc 1
opened pch.cc
closing pch.cc
stat ./pch.h 1
opened ./pch.h
closing ./pch.h
stat ./foo.h 1
stat bar/foo.h 1
opened bar/foo.h
closing bar/foo.h
stat bar/foo2.h 1
opened bar/foo2.h
closing bar/foo2.h
stat pch.cc 1
opened pch.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing /Users/thakis/src/llvm-mono-2/pch.cc
stat ./pch.h 1
opened ./pch.h
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
/FIpch.h client.cc
stat client.cc 1
opened client.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing client.cc
stat ./pch.h 1
opened ./pch.h
stat ./bar/foo.h 1
opened ./bar/foo.h
closing ./bar/foo.h
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 1
opened /Users/thakis/src/llvm-mono-2/bar/foo2.h
closing /Users/thakis/src/llvm-mono-2/bar/foo2.h
stat ./bar/foo2.h 1
opened ./bar/foo2.h
stat ./foo3.h 1
opened ./foo3.h
closing ./foo3.h


Note how ./bar/foo2.h is opened in the 4th-to-last line but never closed.
For comparision, with your patch reverted:

Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
/FIpch.h pch.cc  /Ibar
stat pch.cc 1
opened pch.cc
closing pch.cc
stat ./pch.h 1
opened ./pch.h
closing ./pch.h
stat ./foo.h 1
stat bar/foo.h 1
opened bar/foo.h
closing bar/foo.h
stat bar/foo2.h 1
opened bar/foo2.h
closing bar/foo2.h
stat pch.cc 1
opened pch.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing /Users/thakis/src/llvm-mono-2/pch.cc
stat ./pch.h 1
Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
/FIpch.h client.cc
stat client.cc 1
opened client.cc
stat pch.pch 1
opened pch.pch
closing pch.pch
stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
stat /Users/thakis/src/llvm-mono-2/pch.h 0
stat /Users/thakis/src/llvm-mono-2/pch.cc 0
closing client.cc
stat ./pch.h 1
stat ./bar/foo.h 1
stat ./bar/foo2.h 1
stat ./foo3.h 1
opened ./foo3.h
closing ./foo3.h

On Wed, Jan 23, 2019 at 2:21 PM Nico Weber  wrote:

> On Wed, Jan 23, 2019 at 2:17 PM Nico Weber  wrote:
>
>> Here's what I think is a repro:
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat foo2.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
>> #include "foo.h"
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
>> #include "foo2.h"
>> #include "foo.h"
>>
>> #include "foo2.h"
>> #include "foo.h"
>>
>> Then just build pch.cc with a pch file, and then use it (you can do this
>> on any OS; you could also use the gcc style driver if you prefer it).
>>
>> The following output is with your change reverted, I added
>>
>> fprintf(stderr, "stat %s %d\n", Filename.str().c_str(), openFile);
>>
>> right before the call to getStatValue() in getFile, and
>>
>> if (openFile) fprintf(stderr, "opened %s\n", Filename.str().c_str());
>>   UFE.File = std::move(F);
>>
>> at the very bottom of that function, and changed getBufferForFile() to
>> have
>>
>> if (ShouldCloseOpenFile) {
>> fprintf(stderr, "closing %s\n", Entry->getName().str().c_str());
>>   Entry->closeFile();
>> }
>>
>>
>> The output is:
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>> /FIpch.h pch.cc
>> stat pch.cc 1
>> opened pch.cc
>> closing pch.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> closing ./pch.h
>> stat ./foo.h 1
>> opened ./foo.h
>> closing ./foo.h
>> stat pch.cc 1
>> opened pch.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat /Users/thakis/src/llvm-mono-2/foo.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>> closing /Users/thakis/src/llvm-mono-2/pch.cc
>> stat ./pch.h 1
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>> /FIpch.h client.cc
>> stat client.cc 1

[PATCH] D57112: [ASTTypeTraits] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: sbenza, bkramer, pcc, klimek, hokein.
lebedev.ri added a project: OpenMP.
lebedev.ri added a child revision: D57113: [clang-tidy] openmp-use-default-none 
- a new module and a check.

`OMPClause` is the base class, it is not descendant from **any** other class,
therefore for it to work with e.g. `VariadicDynCastAllOfMatcher<>`, it needs to 
be handled here.

Now, i'm not quite sure what else should be here,
i have simply followed the preexisting, prevalent pattern in these source files.

The test coverage i have will be in the clang-tidy check.


Repository:
  rC Clang

https://reviews.llvm.org/D57112

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

Index: lib/AST/ASTTypeTraits.cpp
===
--- lib/AST/ASTTypeTraits.cpp
+++ lib/AST/ASTTypeTraits.cpp
@@ -37,6 +37,9 @@
   { NKI_None, "Type" },
 #define TYPE(DERIVED, BASE) { NKI_##BASE, #DERIVED "Type" },
 #include "clang/AST/TypeNodes.def"
+  { NKI_None, "OMPClause" },
+#define OPENMP_CLAUSE(TextualSpelling, DERIVED) { NKI_OMPClause, #DERIVED },
+#include "clang/Basic/OpenMPKinds.def"
 };
 
 bool ASTNodeKind::isBaseOf(ASTNodeKind Other, unsigned *Distance) const {
@@ -103,6 +106,19 @@
 #include "clang/AST/TypeNodes.def"
   }
   llvm_unreachable("invalid type kind");
+ }
+
+ASTNodeKind ASTNodeKind::getFromNode(const OMPClause &C) {
+  switch (C.getClauseKind()) {
+#define OPENMP_CLAUSE(Name, Class) \
+case OMPC_##Name: return ASTNodeKind(NKI_##Class);
+#include "clang/Basic/OpenMPKinds.def"
+case OMPC_threadprivate:
+case OMPC_uniform:
+case OMPC_unknown:
+  llvm_unreachable("unexpected OpenMP clause kind");
+  }
+  llvm_unreachable("invalid stmt kind");
 }
 
 void DynTypedNode::print(llvm::raw_ostream &OS,
@@ -151,6 +167,8 @@
 return D->getSourceRange();
   if (const Stmt *S = get())
 return S->getSourceRange();
+  if (const OMPClause *C = get())
+return SourceRange(C->getBeginLoc(), C->getEndLoc());
   return SourceRange();
 }
 
Index: include/clang/AST/ASTTypeTraits.h
===
--- include/clang/AST/ASTTypeTraits.h
+++ include/clang/AST/ASTTypeTraits.h
@@ -18,6 +18,7 @@
 #include "clang/AST/ASTFwd.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/OpenMPClause.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TypeLoc.h"
@@ -58,6 +59,7 @@
   static ASTNodeKind getFromNode(const Decl &D);
   static ASTNodeKind getFromNode(const Stmt &S);
   static ASTNodeKind getFromNode(const Type &T);
+  static ASTNodeKind getFromNode(const OMPClause &C);
   /// \}
 
   /// Returns \c true if \c this and \c Other represent the same kind.
@@ -136,6 +138,9 @@
 NKI_Type,
 #define TYPE(DERIVED, BASE) NKI_##DERIVED##Type,
 #include "clang/AST/TypeNodes.def"
+NKI_OMPClause,
+#define OPENMP_CLAUSE(TextualSpelling, DERIVED) NKI_##DERIVED,
+#include "clang/Basic/OpenMPKinds.def"
 NKI_NumberOfKinds
   };
 
@@ -183,12 +188,15 @@
 KIND_TO_KIND_ID(Decl)
 KIND_TO_KIND_ID(Stmt)
 KIND_TO_KIND_ID(Type)
+KIND_TO_KIND_ID(OMPClause)
 #define DECL(DERIVED, BASE) KIND_TO_KIND_ID(DERIVED##Decl)
 #include "clang/AST/DeclNodes.inc"
 #define STMT(DERIVED, BASE) KIND_TO_KIND_ID(DERIVED)
 #include "clang/AST/StmtNodes.inc"
 #define TYPE(DERIVED, BASE) KIND_TO_KIND_ID(DERIVED##Type)
 #include "clang/AST/TypeNodes.def"
+#define OPENMP_CLAUSE(TextualSpelling, DERIVED) KIND_TO_KIND_ID(DERIVED)
+#include "clang/Basic/OpenMPKinds.def"
 #undef KIND_TO_KIND_ID
 
 inline raw_ostream &operator<<(raw_ostream &OS, ASTNodeKind K) {
@@ -459,6 +467,11 @@
 T, typename std::enable_if::value>::type>
 : public DynCastPtrConverter {};
 
+template 
+struct DynTypedNode::BaseConverter<
+T, typename std::enable_if::value>::type>
+: public DynCastPtrConverter {};
+
 template <>
 struct DynTypedNode::BaseConverter<
 NestedNameSpecifier, void> : public PtrConverter {};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56760: Add a new builtin: __builtin_dynamic_object_size

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D56760#1368054 , @erik.pilkington 
wrote:

> So it seems like the GCC people want to keep `__builtin_object_size` static.


I don't see any evidence of that. Jakub said that modes 0-3 should stay static, 
but that's in line with what we suggested.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56760



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

[PATCH] D57113: [clang-tidy] openmp-use-default-none - a new module and a check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: JonasToth, aaron.ballman, alexfh, xazax.hun, hokein.
lebedev.ri added projects: clang-tools-extra, OpenMP.
Herald added subscribers: guansong, rnkovacs, mgorny.

Finds OpenMP directives that are allowed to contain `default` clause,
but either don't specify it, or the clause is specified but with the kind
other than `none`, and suggests to use `default(none)` clause.

Using `default(none)` clause changes the default variable visibility from
being implicitly determined, and thus forces developer to be explicit about the
desired data scoping for each variable.



Since this seems to be ground-breaking (as in the first one) OpenMP check
in (upstream?) clang-tidy, there is a lot of boilerplate matchers.
I suppose eventually (some of them?) will migrate to be proper AST matchers.

I'm not happy with how `isAllowedToContainClause()` actually takes the
`enum OpenMPClauseKind` value, instead of something nicer, be it either
the actual AST class name (`OMPDefaultClause`), or the matcher 
(`ompDefaultClause`)
that would normally match that node.

Same goes for `ompDefaultClause(hasKind())`, which takes the
`enum OpenMPDefaultClauseKind` value.

Depends on D57112 .


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D57113

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/ClangTidyForceLinker.h
  clang-tidy/openmp/CMakeLists.txt
  clang-tidy/openmp/OpenMPTidyModule.cpp
  clang-tidy/openmp/UseDefaultNoneCheck.cpp
  clang-tidy/openmp/UseDefaultNoneCheck.h
  clang-tidy/plugin/CMakeLists.txt
  clang-tidy/tool/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/openmp-use-default-none.rst
  test/clang-tidy/openmp-use-default-none.cpp

Index: test/clang-tidy/openmp-use-default-none.cpp
===
--- /dev/null
+++ test/clang-tidy/openmp-use-default-none.cpp
@@ -0,0 +1,68 @@
+// RUN: %check_clang_tidy %s openmp-use-default-none %t -- -- -x c++ -fopenmp -fopenmp-version=31
+// RUN: %check_clang_tidy %s openmp-use-default-none %t -- -- -x c   -fopenmp -fopenmp-version=31
+
+////
+// Trivial cases.
+////
+
+// 'for' directive can not have 'default' clause, no diagnostics.
+void t0(const int a) {
+#pragma omp for
+  for (int b = 0; b < a; b++)
+;
+}
+
+// 'parallel' directive can have 'default' clause, but said clause is not
+// specified, diagnosed.
+void t1() {
+#pragma omp parallel
+  ;
+  // CHECK-NOTES: :[[@LINE-2]]:9: warning: OpenMP directive 'parallel' is allowed to contain the 'default' clause, but no 'default' clause is specified. Consider specifying 'default(none)' clause.
+}
+
+// 'parallel' directive can have 'default' clause, and said clause is specified,
+// with 'none' kind, all good.
+void t2() {
+#pragma omp parallel default(none)
+  ;
+}
+
+// 'parallel' directive can have 'default' clause, and said clause is specified,
+// but with 'shared' kind, which is not 'none', diagnose.
+void t3() {
+#pragma omp parallel default(shared)
+  ;
+  // CHECK-NOTES: :[[@LINE-2]]:9: warning: OpenMP directive 'parallel' is allowed to contain the 'default' clause, and 'default' clause exists with 'shared' kind. Consider using 'default(none)' clause instead.
+  // CHECK-NOTES: :[[@LINE-3]]:22: note: Existing 'default' clause is specified here.
+}
+
+////
+// Two directive cases.
+////
+
+// 'parallel' directive can have 'default' clause, but said clause is not
+// specified, diagnosed.
+void t4(const int a) {
+#pragma omp parallel for
+  for (int b = 0; b < a; b++)
+;
+  // CHECK-NOTES: :[[@LINE-3]]:9: warning: OpenMP directive 'parallel for' is allowed to contain the 'default' clause, but no 'default' clause is specified. Consider specifying 'default(none)' clause.
+}
+
+// 'parallel' directive can have 'default' clause, and said clause is specified,
+// with 'none' kind, all good.
+void t5(const int a) {
+#pragma omp parallel for default(none)
+  for (int b = 0; b < a; b++)
+;
+}
+
+// 'parallel' directive can have 'default' clause, and said clause is specified,
+// but with 'shared' kind, which is not 'none', diagnose.
+void t6(const int a) {
+#pragma omp parallel for default(shared)
+  for (int b = 0; b < a; b++)
+;
+  // CHECK-NOTES: :[[@LINE-3]]:9: warning: OpenMP directive 'parallel for' is allowed to contain the 'default' clause, and 'default' clause exists with 'shared' kind. Consider using 'default(none)' clause instead.
+  // CHECK-NOTES: :[[@LINE-4]]:26: note: Existing 'default' clause is specified here.
+}
Index: docs

[PATCH] D57111: Make Clang not crash on calls to destructors on incomplete pointer types

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Upload patches with full context please.
Also the diagnostic seems to be wrong ?
And finally is there a bug report for this ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57111



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

[PATCH] D56561: [Preprocessor] For missing file in framework add note about framework location.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Ping.


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

https://reviews.llvm.org/D56561



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

[PATCH] D57111: Make Clang not crash on calls to destructors on incomplete pointer types

[Roman Zhikharevich via Phabricator via cfe-commits]

rzhikharevich added a comment.

In D57111#1368224 , @riccibruno wrote:

> Upload patches with full context please.
>  Also the diagnostic seems to be wrong ?


I don't think so, the valid code would be 'p->~C()'.

> And finally is there a bug report for this ?

No. Should I file one?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57111



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

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/Expr.h:5099
+  public:
+using iterator_category = std::forward_iterator_tag;
+using value_type = AssociationTy;

Carrying over the conversation from D57098:

>> @aaron.ballman It seems like this should be pretty trivial to make into a 
>> random access iterator, or am I missing something?
> @riccibruno Indeed, at the cost of some boilerplate. I can totally do this 
> but from what I understood the motivation was to use this in ranges, and for 
> this a forward iterator is good enough (although see the next comment)

You are correct, the primary motivation are range-based APIs. However, the 
iterator-based API should still behave in a reasonable manner and some 
algorithms do better with random access iterators than forward only. Even a 
bidirectional iterator would be preferred because I can at least use 
`std::prev()` on it.



Comment at: include/clang/AST/Expr.h:5103
+using reference = AssociationTy;
+using pointer = AssociationTy;
+AssociationIteratorTy() = default;

Carrying over the conversation from D57098:

>> @aaron.ballman Cute, but I suspect this may come back to bite us at some 
>> point. For instance, if someone thinks they're working with a real pointer, 
>> they're likely to expect pointer arithmetic to work when it won't (at least 
>> they'll get compile errors though).
> @riccibruno Hmm, but pointer is just the return type of operator-> no ? Is it 
> actually required to behave like a pointer ? The only requirement I can find 
> is that It->x must be equivalent to (*It).x, which is true here.

I double-checked and you're right, this is not a requirement of the STL.

> Also looking at the requirements for forward iterators I think that this 
> iterator should actually be downgraded to an input iterator, because of the 
> requirement that reference = T&.

My concern is that with the less functional iterators, common algorithms get 
more expensive. For instance, `std::distance()`, `std::advance()` become more 
expensive without random access, and things like `std::prev()` become 
impossible.

It seems like the view this needs to provide is a read-only access to the 
`AssociationTy` objects (because we need to construct them on the fly), but not 
the data contained within them. If the only thing you can get back from the 
iterator is a const pointer/reference/value type, then we could store a local 
"current association" object in the iterator and return a pointer/reference to 
that. WDYT?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57106



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

[PATCH] D57114: Remove Expr sugar decorating the CXXUuidofExpr node.

[Bill Wendling via Phabricator via cfe-commits]

void created this revision.
void added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.

Sugar, like ConstantExpr, causes an infinite expansion of the template object.


Repository:
  rC Clang

https://reviews.llvm.org/D57114

Files:
  lib/Sema/SemaTemplate.cpp
  test/CodeGenCXX/pr40395.cpp


Index: test/CodeGenCXX/pr40395.cpp
===
--- /dev/null
+++ test/CodeGenCXX/pr40395.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -std=c++17 -fms-extensions -emit-llvm %s -o - 
-triple=x86_64-pc-win32
+
+struct _GUID {};
+struct __declspec(uuid("{----}")) B {};
+
+template 
+struct A {
+  virtual void baz() { A(); }
+};
+
+void f() {
+  A<&__uuidof(B)>();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -6308,7 +6308,7 @@
   // -- a predefined __func__ variable
   if (auto *E = Value.getLValueBase().dyn_cast()) {
 if (isa(E)) {
-  Converted = TemplateArgument(ArgResult.get());
+  Converted = TemplateArgument(ArgResult.get()->IgnoreImpCasts());
   break;
 }
 Diag(Arg->getBeginLoc(), diag::err_template_arg_not_decl_ref)


Index: test/CodeGenCXX/pr40395.cpp
===
--- /dev/null
+++ test/CodeGenCXX/pr40395.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -std=c++17 -fms-extensions -emit-llvm %s -o - -triple=x86_64-pc-win32
+
+struct _GUID {};
+struct __declspec(uuid("{----}")) B {};
+
+template 
+struct A {
+  virtual void baz() { A(); }
+};
+
+void f() {
+  A<&__uuidof(B)>();
+}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -6308,7 +6308,7 @@
   // -- a predefined __func__ variable
   if (auto *E = Value.getLValueBase().dyn_cast()) {
 if (isa(E)) {
-  Converted = TemplateArgument(ArgResult.get());
+  Converted = TemplateArgument(ArgResult.get()->IgnoreImpCasts());
   break;
 }
 Diag(Arg->getBeginLoc(), diag::err_template_arg_not_decl_ref)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57111: Make Clang not crash on calls to destructors on incomplete pointer types

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D57111#1368230 , @rzhikharevich 
wrote:

> In D57111#1368224 , @riccibruno 
> wrote:
>
> > Upload patches with full context please.
> >  Also the diagnostic seems to be wrong ?
>
>
> I don't think so, the valid code would be 'p->~C()'.


You are right, my bad!

>> And finally is there a bug report for this ?
> 
> No. Should I file one?

No, it is just so that we can close the corresponding bug report if there is 
one.
Your fix seems reasonable to me. Thanks for the patch!

I guess you will want someone to commit it ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57111



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

[PATCH] D57114: Remove Expr sugar decorating the CXXUuidofExpr node.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:6311
 if (isa(E)) {
-  Converted = TemplateArgument(ArgResult.get());
+  Converted = TemplateArgument(ArgResult.get()->IgnoreImpCasts());
   break;

`IgnoreParenImpCasts()` or are the paren expressions of value?



Comment at: test/CodeGenCXX/pr40395.cpp:1
+// RUN: %clang_cc1 -std=c++17 -fms-extensions -emit-llvm %s -o - 
-triple=x86_64-pc-win32
+

Why is this test in CodeGenCXX instead of SemaCXX?

The RUN line doesn't seem to be testing anything? Should there be a `-verify` 
and `// expected-no-diagnostics` involved?

It might also be a good idea to add some comments that explain the test case.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57114



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

[PATCH] D56760: Add a new builtin: __builtin_dynamic_object_size

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D56760#1368216 , @rsmith wrote:

> In D56760#1368054 , @erik.pilkington 
> wrote:
>
> > So it seems like the GCC people want to keep `__builtin_object_size` static.
>
>
> I don't see any evidence of that. Jakub said that modes 0-3 should stay 
> static, but that's in line with what we suggested.


You'd originally asked:

> Would it make sense to model this as an (optional) extra flag bit for 
> `__builtin_object_size` rather than as a new builtin? It'd seem reasonable to 
> me to ask on the gcc dev list if they'd be interested in such an extension to 
> their builtin -- if not, then we should use a new name, and something like 
> this seems fine to me.

I agree Jakub wants to keep 0-3 static, but I didn't see a preference expressed 
for an extra bit versus `__builtin_dynamic_object_size`. Given this, which 
direction do you want to take?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56760



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

[PATCH] D57111: Make Clang not crash on calls to destructors on incomplete pointer types

[Roman Zhikharevich via Phabricator via cfe-commits]

rzhikharevich added a comment.

In D57111#1368256 , @riccibruno wrote:

> I guess you will want someone to commit it ?


Yes, if the fix is correct.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57111



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

[PATCH] D56760: Add a new builtin: __builtin_dynamic_object_size

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D56760#1368216 , @rsmith wrote:

> I don't see any evidence of that. Jakub said that modes 0-3 should stay 
> static, but that's in line with what we suggested.


I don't think Jakub really said anything about whether we should go with a new 
builtin or use the flag bit. I'm basing this on Jonathan Wakely's comment:

> I know Jakub is concerned about arbitrarily complex expressions, when
>  **__builtin_object_size is supposed to always be efficient and always
>  evaluate at compile time (which would imply the dynamic behaviour
>  should be a separate builtin, if it exists at all)**.

So your call. FWIW I'd prefer the __builtin_object_size spelling too, but it 
doesn't seem like the GCC folks are super crazy about it to me. So it seems 
likely to me that if we implement it it will just be a clang extension for at 
least the medium term (if not permanently). I guess that's fine, so long as the 
GCC people are aware that it would be bad to extend their builtin to use 
`type&4`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56760



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

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D57104#1368072 , @riccibruno wrote:

> In D57104#1368055 , @steveire wrote:
>
> > Splitting the introduction of and porting to `Create` would significantly 
> > reduce the number of files touched by the 'real' change in this commit, and 
> > therefore reduce noise in the commit (following the idea of "do one thing 
> > per commit" to make the code reviewable in the future).
> >
> > However, if you're opposed to that, it's not a hard requirement.
>
>
> To be honest I don't really see the point.


Yes, the `Create` refactor a simple change. The point is to remove the noise of 
the simple change from the complex change so that the complex change becomes 
visible.

It would help reviewers like myself who are less familiar with what refactoring 
for tail allocation involves. I couldn't read your commit and know how to do it 
for another class because this commit is full of noise.

Anyway, if making the code reviewable (also in the future!) like that is not 
important enough, I won't block this one! :)




Comment at: include/clang/AST/Expr.h:5095
 
-  SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; }
-  SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }

Why remove the `LLVM_READONLY` from this class instead of from everywhere in 
the file it is not needed? (in a separate commit, obviously).


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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