[PATCH] D55229: [COFF, ARM64] Make -flto-visibility-public-std a driver and cc1 flag

[Mandeep Singh Grang via Phabricator via cfe-commits]

mgrang added a comment.

I wonder why this flag is called -flto-visibility-public-std. It has nothing to 
do with -flto. While we are at it, does it make sense to rename this flag?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55229



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

[PATCH] D55229: [COFF, ARM64] Make -flto-visibility-public-std a driver and cc1 flag

[Mandeep Singh Grang via Phabricator via cfe-commits]

mgrang created this revision.
mgrang added reviewers: rnk, mstorsjo, efriedma, TomTan.
Herald added subscribers: dexonsmith, kristof.beyls, inglorion, javed.absar, 
mehdi_amini.

Clang currently imports certain functions (like __imp__CxxThrowException) even 
when statically linking.
Whereas MSVC statically links them (like __CxxThrowException). In clang, 
function linkage is controlled via /MT 
flag which is a clang-cl flag. This flag gets expanded to 
-flto-visibility-public-std which is a -cc1 option.
So from clang driver there is no way to statically link these functions. This 
patch makes -flto-visibility-public-std a
 driver flag which can be used to control function linkage.


Repository:
  rC Clang

https://reviews.llvm.org/D55229

Files:
  CodeGen/arm64-microsoft-symbol-linkage.cpp
  Driver/ToolChains/Clang.cpp
  clang/Driver/CC1Options.td
  clang/Driver/Options.td


Index: CodeGen/arm64-microsoft-symbol-linkage.cpp
===
--- /dev/null
+++ CodeGen/arm64-microsoft-symbol-linkage.cpp
@@ -0,0 +1,29 @@
+// REQUIRES: aarch64-registered-target
+
+// RUN: %clangxx -target aarch64-windows \
+// RUN: -fcxx-exceptions -emit-obj -c -o - %s \
+// RUN: | llvm-objdump -syms - 2>&1 | FileCheck %s --check-prefix LINK-DYN
+
+// RUN: %clangxx -target aarch64-windows -flto-visibility-public-std \
+// RUN: -fcxx-exceptions -emit-obj -c -o - %s \
+// RUN: | llvm-objdump -syms - 2>&1 | FileCheck %s --check-prefix LINK-STAT
+
+void foo1() { throw 1; }
+// CHECK-LABEL: foo1
+// LINK-DYN: __imp__CxxThrowException
+// LINK-STAT-NOT: __imp__CxxThrowException
+
+void bar();
+void foo2() noexcept(true) { bar(); }
+// CHECK-LABEL: foo2
+// LINK-DYN: __imp___std_terminate
+// LINK-STAT-NOT: __imp___std_terminate
+
+struct A {};
+struct B { virtual void f(); };
+struct C : A, virtual B {};
+struct T {};
+T *foo3() { return dynamic_cast((C *)0); }
+// CHECK-LABEL: foo3
+// LINK-DYN: __imp___RTDynamicCast
+// LINK-STAT-NOT: __imp___RTDynamicCast
Index: Driver/ToolChains/Clang.cpp
===
--- Driver/ToolChains/Clang.cpp
+++ Driver/ToolChains/Clang.cpp
@@ -5210,6 +5210,9 @@
TC.useIntegratedAs()))
 CmdArgs.push_back("-faddrsig");
 
+  if (Args.hasArg(options::OPT_flto_visibility_public_std))
+CmdArgs.push_back("-flto-visibility-public-std");
+
   // Finally add the compile command to the compilation.
   if (Args.hasArg(options::OPT__SLASH_fallback) &&
   Output.getType() == types::TY_Object &&
Index: clang/Driver/Options.td
===
--- clang/Driver/Options.td
+++ clang/Driver/Options.td
@@ -825,6 +825,9 @@
 def fcreate_profile : Flag<["-"], "fcreate-profile">, Group;
 def fcxx_exceptions: Flag<["-"], "fcxx-exceptions">, Group,
   HelpText<"Enable C++ exceptions">, Flags<[CC1Option]>;
+def flto_visibility_public_std:
+Flag<["-"], "flto-visibility-public-std">, Group, 
Flags<[CC1Option]>,
+HelpText<"Use public LTO visibility for classes in std and stdext 
namespaces">;
 def fcxx_modules : Flag <["-"], "fcxx-modules">, Group,
   Flags<[DriverOption]>;
 def fdebug_pass_arguments : Flag<["-"], "fdebug-pass-arguments">, 
Group;
Index: clang/Driver/CC1Options.td
===
--- clang/Driver/CC1Options.td
+++ clang/Driver/CC1Options.td
@@ -345,9 +345,6 @@
 def fprofile_instrument_use_path_EQ :
 Joined<["-"], "fprofile-instrument-use-path=">,
 HelpText<"Specify the profile path in PGO use compilation">;
-def flto_visibility_public_std:
-Flag<["-"], "flto-visibility-public-std">,
-HelpText<"Use public LTO visibility for classes in std and stdext 
namespaces">;
 def flto_unit: Flag<["-"], "flto-unit">,
 HelpText<"Emit IR to support LTO unit features (CFI, whole program vtable 
opt)">;
 def fno_lto_unit: Flag<["-"], "fno-lto-unit">;


Index: CodeGen/arm64-microsoft-symbol-linkage.cpp
===
--- /dev/null
+++ CodeGen/arm64-microsoft-symbol-linkage.cpp
@@ -0,0 +1,29 @@
+// REQUIRES: aarch64-registered-target
+
+// RUN: %clangxx -target aarch64-windows \
+// RUN: -fcxx-exceptions -emit-obj -c -o - %s \
+// RUN: | llvm-objdump -syms - 2>&1 | FileCheck %s --check-prefix LINK-DYN
+
+// RUN: %clangxx -target aarch64-windows -flto-visibility-public-std \
+// RUN: -fcxx-exceptions -emit-obj -c -o - %s \
+// RUN: | llvm-objdump -syms - 2>&1 | FileCheck %s --check-prefix LINK-STAT
+
+void foo1() { throw 1; }
+// CHECK-LABEL: foo1
+// LINK-DYN: __imp__CxxThrowException
+// LINK-STAT-NOT: __imp__CxxThrowException
+
+void bar();
+void foo2() noexcept(true) { bar(); }
+// CHECK-LABEL: foo2
+// LINK-DYN: __imp___std_terminate
+// LINK-STAT-NOT: __imp___std_terminate
+
+struct A {};
+struct B { virtual void f(); };
+struct C : A, virtual B {};
+struct T {};
+T *foo3() { return dynamic_cast((C *)0); }
+// 

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Cameron McInally via Phabricator via cfe-commits]

cameron.mcinally added inline comments.



Comment at: include/llvm/IR/IRBuilder.h:1244
   }
 
   /// Copy fast-math-flags from an instruction rather than using the builder's

I think you can forgo the `else {` in these functions since the if branch 
returns immediately.


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

https://reviews.llvm.org/D53157



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

[clang-tools-extra] r348176 - Fix compilation failure on Windows.

[Zachary Turner via cfe-commits]

Author: zturner
Date: Mon Dec  3 11:59:00 2018
New Revision: 348176

URL: http://llvm.org/viewvc/llvm-project?rev=348176=rev
Log:
Fix compilation failure on Windows.

This was introduced earlier but apparently used an incorrect
class name so it doesn't compile on Windows.

Modified:
clang-tools-extra/trunk/clangd/FSProvider.cpp

Modified: clang-tools-extra/trunk/clangd/FSProvider.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FSProvider.cpp?rev=348176=348175=348176=diff
==
--- clang-tools-extra/trunk/clangd/FSProvider.cpp (original)
+++ clang-tools-extra/trunk/clangd/FSProvider.cpp Mon Dec  3 11:59:00 2018
@@ -76,7 +76,7 @@ clang::clangd::RealFileSystemProvider::g
 // FIXME: Try to use a similar approach in Sema instead of relying on
 //propagation of the 'isVolatile' flag through all layers.
 #ifdef _WIN32
-  return new VolatileFSProvider(vfs::getRealFileSystem());
+  return new VolatileFileSystem(vfs::getRealFileSystem());
 #else
   return vfs::getRealFileSystem();
 #endif


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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> `int8` ? Did you mean `int8_t` or am I missing somthing ?

Your right, but the solution I wrote did actually not work anyway.. I just 
specialized `std::hash<>` now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54737



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

[clang-tools-extra] r348172 - [clang-tidy] Fix unordered_map failure with specializing std::hash<> and remove previous wrong attempt at doing so

[Jonas Toth via cfe-commits]

Author: jonastoth
Date: Mon Dec  3 11:41:04 2018
New Revision: 348172

URL: http://llvm.org/viewvc/llvm-project?rev=348172=rev
Log:
[clang-tidy] Fix unordered_map failure with specializing std::hash<> and remove 
previous wrong attempt at doing so

Modified:
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h

Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp?rev=348172=348171=348172=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp Mon 
Dec  3 11:41:04 2018
@@ -47,8 +47,7 @@ static llvm::Optional get
 static const std::pair &
 getInverseForScale(DurationScale Scale) {
   static const std::unordered_map,
-  std::hash>
+  std::pair>
   InverseMap(
   {{DurationScale::Hours,
 std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")},

Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h?rev=348172=348171=348172=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h Mon Dec  3 
11:41:04 2018
@@ -27,7 +27,26 @@ enum class DurationScale : std::int8_t {
   Microseconds,
   Nanoseconds,
 };
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
 
+namespace std {
+template <> struct hash<::clang::tidy::abseil::DurationScale> {
+  using argument_type = ::clang::tidy::abseil::DurationScale;
+  using underlying_type = std::underlying_type::type;
+  using result_type = std::hash::result_type;
+
+  result_type operator()(const argument_type ) const {
+std::hash hasher;
+return hasher(static_cast(arg));
+  }
+};
+} // namespace std
+
+namespace clang {
+namespace tidy {
+namespace abseil {
 /// Given a `Scale`, return the appropriate factory function call for
 /// constructing a `Duration` for that scale.
 llvm::StringRef getFactoryForScale(DurationScale Scale);


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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D54737#1317128 , @JonasToth wrote:

> I had to revert and recommitted in rCTE348169 
> . `std::unordered_map Something, ...>` does not work, as `std::hash` is not specialized for it. 
> This behaviour seems to work for some compilers, but some not. I had to 
> google myself a bit for the best solution, but now I specialized the 
> hash-function to `std::hash()` in `unordered_set` which worked for 
> me locally and was suggested. Lets see ;)


`int8` ? Did you mean `int8_t` or am I missing somthing ?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54737



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I had to revert and recommitted in rCTE348169 
. `std::unordered_map` does not work, as `std::hash` is not specialized for it. This 
behaviour seems to work for some compilers, but some not. I had to google 
myself a bit for the best solution, but now I specialized the hash-function to 
`std::hash()` in `unordered_set` which worked for me locally and was 
suggested. Lets see ;)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54737



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

[clang-tools-extra] r348169 - [clang-tidy] Recommit: Add the abseil-duration-comparison check

[Jonas Toth via cfe-commits]

Author: jonastoth
Date: Mon Dec  3 11:22:08 2018
New Revision: 348169

URL: http://llvm.org/viewvc/llvm-project?rev=348169=rev
Log:
[clang-tidy] Recommit: Add the abseil-duration-comparison check

Summary:
This check finds instances where Duration values are being converted to a 
numeric value in a comparison expression, and suggests that the conversion 
happen on the other side of the expression to a Duration.  See documentation 
for examples.

This also shuffles some code around so that the new check may perform in sone 
step simplifications also caught by other checks.
Compilation is unbroken, because the hash-function is now directly
specified for std::unordered_map, as 'enum class' does not compile as
key (seamingly only on some compilers).

Patch by hwright.

Reviewers: aaron.ballman, JonasToth, alexfh, hokein

Reviewed By: JonasToth

Subscribers: sammccall, Eugene.Zelenko, xazax.hun, cfe-commits, mgorny

Tags: #clang-tools-extra

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

Added:
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h

clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst
clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp?rev=348169=348168=348169=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp Mon Dec  3 
11:22:08 2018
@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "DurationComparisonCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
@@ -27,6 +28,8 @@ namespace abseil {
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories ) override {
+CheckFactories.registerCheck(
+"abseil-duration-comparison");
 CheckFactories.registerCheck(
 "abseil-duration-division");
 CheckFactories.registerCheck(

Modified: clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt?rev=348169=348168=348169=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt Mon Dec  3 
11:22:08 2018
@@ -2,9 +2,11 @@ set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  DurationComparisonCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
+  DurationRewriter.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp

Added: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp?rev=348169=auto
==
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp 
(added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp Mon 
Dec  3 11:22:08 2018
@@ -0,0 +1,165 @@
+//===--- DurationComparisonCheck.cpp - clang-tidy 
-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "DurationComparisonCheck.h"
+#include "DurationRewriter.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
+/// return its `DurationScale`, or `None` if a match is not found.
+static 

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Cameron McInally via Phabricator via cfe-commits]

cameron.mcinally added a comment.

In D53157#1316860 , @uweigand wrote:

> In my reading of the standard text, there is no special provision for library 
> code.  This means that in general, calling any library function while running 
> with nondefault trap settings is undefined behavior, unless the library was 
> itself compiled with FENV_ACCESS ON.  There does not even appear to be any 
> requirement for the C standard library functions to have been compiled with 
> FENV_ACCESS ON, as far as I can see ...


I'll take this conversation to llvm-dev in the near future. I don't want to 
detract from Kevin's work.

Apologies for highjacking this Diff, Kevin.


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

https://reviews.llvm.org/D53157



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

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[Pierre van Houtryve via Phabricator via cfe-commits]

Pierre-vh added a comment.

In D55226#1317083 , @george.karpenkov 
wrote:

> Thank you for the fix, but how far can the pattern matching go? Seems easy 
> enough to think of cases not covered by the above.
>  In any case, the fix looks good.


Hey,

Sadly I'm not experienced enough to think of every case that should pass this 
check, so I limited myself to just fixing the bug.
Can't we totally remove the outer if so we allow every `Target` expression that 
has a `ConstantArrayType` to pass this check?

Thank you for your time!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55226



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

[PATCH] D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Assuming this patch were to go in as-is (which it probably won't, based on the 
feedback, but let's just pretend), that would avoid having to explicitly update 
how many callsites?

What I'm wondering is, how hard would it be to just update the call-sites?

It looks like what you are really trying to do is avoid having to pass 
"IsVolatile" for many call-sites.  But to be honest, IsVolatile exactly 
describes this situation.  You have a file, and another program has the same 
file and it can change it out from underneath you.  That exactly describes the 
meaning of `IsVolatile`.  So, conceptually it makes sense to just find all the 
call-sites where this matters and pass `IsVolatile=true`.  Is there some reason 
we can't just do that here?  Are there too many?


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

https://reviews.llvm.org/D54995



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

[PATCH] D55222: [AST] Assert that no statement/expression class is polymorphic

[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!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55222



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

[PATCH] D55212: Handle alloc_size attribute on function pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/Attr.td:1072
 def AllocSize : InheritableAttr {
   let Spellings = [GCC<"alloc_size">];
+  let Subjects = SubjectList<[HasFunctionProto]>;

arichardson wrote:
> aaron.ballman wrote:
> > Does GCC support writing `alloc_size` on function pointers?
> Yes it does and it seems to be used by some projects. I first discovered this 
> while compiling libxml2: 
> https://github.com/GNOME/libxml2/blob/35e83488505d501864826125cfe6a7950d6cba78/include/libxml/xmlmemory.h#L66
Parsed and ignored is different than supported. For instance, I can't seem to 
get GCC to produce different behavior here: https://godbolt.org/z/MI5k_m

Am I missing something?



Comment at: test/Sema/alloc-size.c:46
+// This typedef applies the alloc_size to the pointer to the function pointer 
and should not be allowed
+void *(**__attribute__((alloc_size(1, 2))) * allocator_function_typdef4)(int, 
int); // expected-warning{{'alloc_size' attribute only applies to non-K 
functions}}

arichardson wrote:
> aaron.ballman wrote:
> > What should happen in these situations?
> > ```
> > typedef void * (__attribute__((alloc_size(1))) * 
> > my_malloc_fn_pointer_type)(int);
> > typedef void * (* my_other_malloc_fn_pointer_type)(int);
> > 
> > void *fn(int i);
> > __attribute__((alloc_size(1))) void *fn2(int i);
> > 
> > int main() {
> >   my_malloc_fn_pointer_type f = fn; // ???
> >   my_other_malloc_fn_pointer_type f2 = fn2; // ???
> > }
> > ```
> > Should this code do something special?
> > ```
> > typedef void * (__attribute__((alloc_size(1))) * 
> > my_malloc_fn_pointer_type)(int);
> > typedef void * (* my_other_malloc_fn_pointer_type)(int);
> > 
> > void overloaded(my_malloc_fn_pointer_type fn);
> > void overloaded(my_other_malloc_fn_pointer_type fn);
> > 
> > void *fn(int i);
> > __attribute__((alloc_size(1))) void *fn2(int i);
> > 
> > int main() {
> >   overloaded(fn);
> >   overloaded(fn2);
> > }
> > 
> > ```
> If I define two overloaded functions that only differ on the attribute GCC 
> gives me the following error:
> ```
> :14:6: error: redefinition of 'overloaded'
> 
>14 | void overloaded(my_other_malloc_fn_pointer_type fn) {
> 
>   |  ^~
> 
> :11:6: note: previous definition of 'overloaded' was here
> 
>11 | void overloaded(my_malloc_fn_pointer_type fn) {
> 
>   |  ^~
> ```
> Assigning function pointers with and without the attribute seems to work just 
> fine:
> https://godbolt.org/z/-i5zUK
Great, that's what I was hoping to hear. Can you add a C++ tests for both of 
those? The C test is a good start, but C++ is more finicky about type identity.



Comment at: test/Sema/alloc-size.c:48-49
+
+
+
+// We should not be warning when assigning function pointers with and without 
the alloc size attribute

Spurious newlines can be removed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55212



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

[PATCH] D55222: [AST] Assert that no statement/expression class is polymorphic

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 176435.
riccibruno added a comment.

Forgot a space in the error message


Repository:
  rC Clang

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

https://reviews.llvm.org/D55222

Files:
  lib/AST/Stmt.cpp


Index: lib/AST/Stmt.cpp
===
--- lib/AST/Stmt.cpp
+++ lib/AST/Stmt.cpp
@@ -76,6 +76,14 @@
   return getStmtInfoTableEntry((StmtClass) StmtBits.sClass).Name;
 }
 
+// Check that no statement / expression class is polymorphic. LLVM style RTTI
+// should be used instead. If absolutely needed an exception can still be added
+// here by defining the appropriate macro (but please don't do this).
+#define STMT(CLASS, PARENT) \
+  static_assert(!std::is_polymorphic::value, \
+#CLASS " should not be polymorphic!");
+#include "clang/AST/StmtNodes.inc"
+
 void Stmt::PrintStats() {
   // Ensure the table is primed.
   getStmtInfoTableEntry(Stmt::NullStmtClass);


Index: lib/AST/Stmt.cpp
===
--- lib/AST/Stmt.cpp
+++ lib/AST/Stmt.cpp
@@ -76,6 +76,14 @@
   return getStmtInfoTableEntry((StmtClass) StmtBits.sClass).Name;
 }
 
+// Check that no statement / expression class is polymorphic. LLVM style RTTI
+// should be used instead. If absolutely needed an exception can still be added
+// here by defining the appropriate macro (but please don't do this).
+#define STMT(CLASS, PARENT) \
+  static_assert(!std::is_polymorphic::value, \
+#CLASS " should not be polymorphic!");
+#include "clang/AST/StmtNodes.inc"
+
 void Stmt::PrintStats() {
   // Ensure the table is primed.
   getStmtInfoTableEntry(Stmt::NullStmtClass);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r348165 - Revert "[clang-tidy] Add the abseil-duration-comparison check"

[Jonas Toth via cfe-commits]

Author: jonastoth
Date: Mon Dec  3 10:59:27 2018
New Revision: 348165

URL: http://llvm.org/viewvc/llvm-project?rev=348165=rev
Log:
Revert "[clang-tidy] Add the abseil-duration-comparison check"

This commit broke buildbots and needs adjustments.

Removed:
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h

clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst
clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp?rev=348165=348164=348165=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp Mon Dec  3 
10:59:27 2018
@@ -10,7 +10,6 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
-#include "DurationComparisonCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
@@ -28,8 +27,6 @@ namespace abseil {
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories ) override {
-CheckFactories.registerCheck(
-"abseil-duration-comparison");
 CheckFactories.registerCheck(
 "abseil-duration-division");
 CheckFactories.registerCheck(

Modified: clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt?rev=348165=348164=348165=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt Mon Dec  3 
10:59:27 2018
@@ -2,11 +2,9 @@ set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
-  DurationComparisonCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
-  DurationRewriter.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp

Removed: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp?rev=348164=auto
==
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp 
(removed)
@@ -1,164 +0,0 @@
-//===--- DurationComparisonCheck.cpp - clang-tidy 
-===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===--===//
-
-#include "DurationComparisonCheck.h"
-#include "DurationRewriter.h"
-#include "clang/AST/ASTContext.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Tooling/FixIt.h"
-
-using namespace clang::ast_matchers;
-
-namespace clang {
-namespace tidy {
-namespace abseil {
-
-/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
-/// return its `DurationScale`, or `None` if a match is not found.
-static llvm::Optional getScaleForInverse(llvm::StringRef Name) {
-  static const llvm::DenseMap ScaleMap(
-  {{"ToDoubleHours", DurationScale::Hours},
-   {"ToInt64Hours", DurationScale::Hours},
-   {"ToDoubleMinutes", DurationScale::Minutes},
-   {"ToInt64Minutes", DurationScale::Minutes},
-   {"ToDoubleSeconds", DurationScale::Seconds},
-   {"ToInt64Seconds", DurationScale::Seconds},
-   {"ToDoubleMilliseconds", DurationScale::Milliseconds},
-   {"ToInt64Milliseconds", DurationScale::Milliseconds},
-   {"ToDoubleMicroseconds", DurationScale::Microseconds},
-   {"ToInt64Microseconds", DurationScale::Microseconds},
-   {"ToDoubleNanoseconds", DurationScale::Nanoseconds},
-   {"ToInt64Nanoseconds", 

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Kevin P. Neal via Phabricator via cfe-commits]

kpn updated this revision to Diff 176432.
kpn added a comment.

I've changed the patch so that calls to CreateFAdd() et al will give you 
constrained intrinsics if they are enabled. This required adding functions to 
enable/disable constrained-as-default plus calls to deal with the rounding mode 
and exception handling for contrained intrinsics.

I've left the independent create constrained functions to make it dead easy to 
move them out of the header and avoid inlining bloat.

I'm not dealing with any fast math flags or metadata currently. I'm not sure 
they make sense in a constrained context, or at least I haven't seen anyone 
argue they must be in place for an initial implementation.


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

https://reviews.llvm.org/D53157

Files:
  include/llvm/IR/IRBuilder.h
  unittests/IR/IRBuilderTest.cpp

Index: unittests/IR/IRBuilderTest.cpp
===
--- unittests/IR/IRBuilderTest.cpp
+++ unittests/IR/IRBuilderTest.cpp
@@ -123,6 +123,100 @@
   EXPECT_FALSE(II->hasNoNaNs());
 }
 
+TEST_F(IRBuilderTest, ConstrainedFP) {
+  IRBuilder<> Builder(BB);
+  Value *V;
+  CallInst *Call;
+  IntrinsicInst *II;
+
+  V = Builder.CreateLoad(GV);
+
+  Call = Builder.CreateConstrainedFAdd(V, V);
+  II = cast(Call);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_fadd);
+
+  Call = Builder.CreateConstrainedFSub(V, V);
+  II = cast(Call);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_fsub);
+
+  Call = Builder.CreateConstrainedFMul(V, V);
+  II = cast(Call);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_fmul);
+
+  Call = Builder.CreateConstrainedFDiv(V, V);
+  II = cast(Call);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_fdiv);
+
+  Call = Builder.CreateConstrainedFRem(V, V);
+  II = cast(Call);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_frem);
+
+  // Now see if we get constrained intrinsics instead of non-constrained
+  // instructions.
+  Builder.setIsConstrainedFP(true);
+
+  V = Builder.CreateFAdd(V, V);
+  ASSERT_TRUE(isa(V));
+  II = cast(V);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_fadd);
+
+  V = Builder.CreateFSub(V, V);
+  ASSERT_TRUE(isa(V));
+  II = cast(V);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_fsub);
+
+  V = Builder.CreateFMul(V, V);
+  ASSERT_TRUE(isa(V));
+  II = cast(V);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_fmul);
+  
+  V = Builder.CreateFDiv(V, V);
+  ASSERT_TRUE(isa(V));
+  II = cast(V);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_fdiv);
+  
+  V = Builder.CreateFRem(V, V);
+  ASSERT_TRUE(isa(V));
+  II = cast(V);
+  EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_frem);
+
+  // Verify the codepaths for setting and overriding the default metadata.
+  MDNode *ExceptStr = MDNode::get(Builder.getContext(), 
+  MDString::get(Builder.getContext(), 
+"fpexcept.strict"));
+  MDNode *ExceptIgn = MDNode::get(Builder.getContext(), 
+  MDString::get(Builder.getContext(), 
+"fpexcept.ignore"));
+  MDNode *RoundDyn = MDNode::get(Builder.getContext(), 
+ MDString::get(Builder.getContext(),
+   "round.dynamic"));
+  MDNode *RoundUp = MDNode::get(Builder.getContext(), 
+MDString::get(Builder.getContext(),
+  "round.upward"));
+
+  V = Builder.CreateFAdd(V, V);
+  ASSERT_TRUE(isa(V));
+  auto *CII = cast(V);
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebStrict);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmDynamic);
+
+  Builder.setDefaultConstrainedExcept(ExceptIgn);
+  Builder.setDefaultConstrainedRounding(RoundUp);
+  V = Builder.CreateFAdd(V, V);
+  CII = cast(V);
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebIgnore);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmUpward);
+
+  // Now override the defaults.
+  Call = Builder.CreateConstrainedFAdd(V, V, nullptr, "", RoundDyn, ExceptStr);
+  CII = cast(Call);
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebStrict);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmDynamic);
+
+  Builder.CreateRetVoid();
+  EXPECT_FALSE(verifyModule(*M));
+}
+
 TEST_F(IRBuilderTest, Lifetime) {
   IRBuilder<> Builder(BB);
   AllocaInst *Var1 = Builder.CreateAlloca(Builder.getInt8Ty());
Index: include/llvm/IR/IRBuilder.h
===
--- include/llvm/IR/IRBuilder.h
+++ include/llvm/IR/IRBuilder.h
@@ -97,13 +97,18 @@

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[George Karpenkov via Phabricator via cfe-commits]

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

Thank you for the fix, but how far can the pattern matching go? Seems easy 
enough to think of cases not covered by the above.
In any case, the fix looks good.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55226



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

[PATCH] D55225: [AST] Assert that no type class is polymorphic

[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!




Comment at: lib/AST/Type.cpp:299
+  static_assert(!std::is_polymorphic::value, \
+#CLASS "Type should not be polymorphic!");
+#include "clang/AST/TypeNodes.def"

riccibruno wrote:
> aaron.ballman wrote:
> > This will squish your class name and error text together. I think you need 
> > a whitespace before "Type" and make that lowercase.
> This is on purpose, since what is substituted into
> `CLASS` is the name of the type class, without the "Type",
> eg: for `FunctionProtoType`, `CLASS` will be `FunctionProto`
> and we want to stick the `Type` to it so that the error message will
> be "FunctionProtoType should not ".
Ah! Thank you for pointing that out.



Comment at: lib/AST/Type.cpp:301
+#include "clang/AST/TypeNodes.def"
+
 QualType Type::getLocallyUnqualifiedSingleStepDesugaredType() const {

riccibruno wrote:
> aaron.ballman wrote:
> > I'd feel more comfortable if there was a `#undef TYPE` here.
> I can certainly add one but it is already done in `TypeNodes.def`.
Ah, nm then -- I didn't see we had that at the bottom of that file.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55225



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

[PATCH] D55225: [AST] Assert that no type class is polymorphic

[Bruno Ricci via Phabricator via cfe-commits]

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



Comment at: lib/AST/Type.cpp:299
+  static_assert(!std::is_polymorphic::value, \
+#CLASS "Type should not be polymorphic!");
+#include "clang/AST/TypeNodes.def"

aaron.ballman wrote:
> This will squish your class name and error text together. I think you need a 
> whitespace before "Type" and make that lowercase.
This is on purpose, since what is substituted into
`CLASS` is the name of the type class, without the "Type",
eg: for `FunctionProtoType`, `CLASS` will be `FunctionProto`
and we want to stick the `Type` to it so that the error message will
be "FunctionProtoType should not ".



Comment at: lib/AST/Type.cpp:301
+#include "clang/AST/TypeNodes.def"
+
 QualType Type::getLocallyUnqualifiedSingleStepDesugaredType() const {

aaron.ballman wrote:
> I'd feel more comfortable if there was a `#undef TYPE` here.
I can certainly add one but it is already done in `TypeNodes.def`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55225



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348161: [clang-tidy] Add the abseil-duration-comparison 
check (authored by JonasToth, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54737?vs=176156=176431#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54737

Files:
  clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h
  clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
  clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp

Index: clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "DurationComparisonCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
@@ -27,6 +28,8 @@
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories ) override {
+CheckFactories.registerCheck(
+"abseil-duration-comparison");
 CheckFactories.registerCheck(
 "abseil-duration-division");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
@@ -2,9 +2,11 @@
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  DurationComparisonCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
+  DurationRewriter.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
@@ -8,6 +8,7 @@
 //===--===//
 
 #include "DurationFactoryScaleCheck.h"
+#include "DurationRewriter.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Tooling/FixIt.h"
@@ -18,20 +19,6 @@
 namespace tidy {
 namespace abseil {
 
-namespace {
-
-// Potential scales of our inputs.
-enum class DurationScale {
-  Hours,
-  Minutes,
-  Seconds,
-  Milliseconds,
-  Microseconds,
-  Nanoseconds,
-};
-
-} // namespace
-
 // Given the name of a duration factory function, return the appropriate
 // `DurationScale` for that factory.  If no factory can be found for
 // `FactoryName`, return `None`.
@@ -129,39 +116,14 @@
   return llvm::None;
 }
 
-// Given a `Scale`, return the appropriate factory function call for
-// constructing a `Duration` for that scale.
-static llvm::StringRef GetFactoryForScale(DurationScale Scale) {
-  switch (Scale) {
-  case DurationScale::Hours:
-return "absl::Hours";
-  case DurationScale::Minutes:
-return "absl::Minutes";
-  case DurationScale::Seconds:
-return "absl::Seconds";
-  case DurationScale::Milliseconds:
-return "absl::Milliseconds";
-  case DurationScale::Microseconds:
-return "absl::Microseconds";
-  case DurationScale::Nanoseconds:
-return "absl::Nanoseconds";
-  }
-  llvm_unreachable("unknown scaling factor");
-}
-
 void DurationFactoryScaleCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
   callExpr(
-  callee(functionDecl(
- hasAnyName("::absl::Nanoseconds", "::absl::Microseconds",
-"::absl::Milliseconds", "::absl::Seconds",
-"::absl::Minutes", "::absl::Hours"))
- .bind("call_decl")),
+  callee(functionDecl(DurationFactoryFunction()).bind("call_decl")),
   hasArgument(
   0,
   ignoringImpCasts(anyOf(
-  

[clang-tools-extra] r348161 - [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via cfe-commits]

Author: jonastoth
Date: Mon Dec  3 10:35:56 2018
New Revision: 348161

URL: http://llvm.org/viewvc/llvm-project?rev=348161=rev
Log:
[clang-tidy] Add the abseil-duration-comparison check

Summary:
This check finds instances where Duration values are being converted to a 
numeric value in a comparison expression, and suggests that the conversion 
happen on the other side of the expression to a Duration.  See documentation 
for examples.

This also shuffles some code around so that the new check may perform in sone 
step simplifications also caught by other checks.

Patch by hwright.

Reviewers: aaron.ballman, JonasToth, alexfh, hokein

Reviewed By: JonasToth

Subscribers: sammccall, Eugene.Zelenko, xazax.hun, cfe-commits, mgorny

Tags: #clang-tools-extra

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

Added:
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.h
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h

clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-comparison.rst
clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryFloatCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp?rev=348161=348160=348161=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp Mon Dec  3 
10:35:56 2018
@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "DurationComparisonCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
@@ -27,6 +28,8 @@ namespace abseil {
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories ) override {
+CheckFactories.registerCheck(
+"abseil-duration-comparison");
 CheckFactories.registerCheck(
 "abseil-duration-division");
 CheckFactories.registerCheck(

Modified: clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt?rev=348161=348160=348161=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt Mon Dec  3 
10:35:56 2018
@@ -2,9 +2,11 @@ set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  DurationComparisonCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
+  DurationRewriter.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp

Added: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp?rev=348161=auto
==
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp 
(added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp Mon 
Dec  3 10:35:56 2018
@@ -0,0 +1,164 @@
+//===--- DurationComparisonCheck.cpp - clang-tidy 
-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "DurationComparisonCheck.h"
+#include "DurationRewriter.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
+/// return its `DurationScale`, or `None` if a match is not found.
+static llvm::Optional getScaleForInverse(llvm::StringRef Name) {
+  static const llvm::DenseMap ScaleMap(
+  {{"ToDoubleHours", DurationScale::Hours},
+   {"ToInt64Hours", 

[PATCH] D55225: [AST] Assert that no type class is polymorphic

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/Type.cpp:299
+  static_assert(!std::is_polymorphic::value, \
+#CLASS "Type should not be polymorphic!");
+#include "clang/AST/TypeNodes.def"

This will squish your class name and error text together. I think you need a 
whitespace before "Type" and make that lowercase.



Comment at: lib/AST/Type.cpp:301
+#include "clang/AST/TypeNodes.def"
+
 QualType Type::getLocallyUnqualifiedSingleStepDesugaredType() const {

I'd feel more comfortable if there was a `#undef TYPE` here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55225



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

[PATCH] D55221: [AST] Make ArrayTypeTraitExpr non-polymorphic.

[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!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55221



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

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Hi russell

thank you for the patch! As I am not a clang-format reviewers these are only 
general things, and Nits anyway ;)
Hope the reviewers I added will evaluate better.




Comment at: lib/Format/WhitespaceManager.cpp:54
   Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
-  Changes.push_back(Change(Tok, /*CreateReplacement=*/true, 
Tok.WhitespaceRange,
-   Spaces, StartOfTokenColumn, Newlines, "", "",
-   InPPDirective && !Tok.IsFirst,
+  auto OriginalWhitespaceRange = Tok.WhitespaceRange;
+

is this a SourceRange? Please spell out the type, as there are different kind 
of ranges.



Comment at: unittests/Format/FormatTest.cpp:9159
"}"));
+
   Tab.AlignConsecutiveAssignments = true;

Spurious change.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54881



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

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[Pierre van Houtryve via Phabricator via cfe-commits]

Pierre-vh created this revision.
Pierre-vh added reviewers: dcoughlin, MaskRay.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
a.sidorin, szepet, baloghadamsoftware.
Herald added a reviewer: george.karpenkov.

Fix for the bug n°39792: False positive on strcpy targeting struct member
Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=39792

I fixed it by replacing the use of `dyn_cast` by two `isa`s to check if 
`Target` is a `DeclRefExpr` or a `MemberExpr`.
The removal of the `DeclRef` variable seems to be meaningless because the only 
place where the `DeclRef` variable was used is one line below, and it was used 
to call a method which is inherited from Expr. 
Thus, replacing the only use of `DeclRef` by `Target` should have no effect.

I also added a small test for this bugfix in 
`test/Analysis/security-syntax-checks.m`

**Note:** I think we can completely remove the outer `if 
(isa(Target) || isa(Target))`, no? Why should we only 
allow `DeclRefExpr`s to pass this check?

**PS:** This is my first contribution ever to CLang (or any other open source 
project), so I'm totally open to feedback, even if it's harsh.

Thank you for your attention!


Repository:
  rC Clang

https://reviews.llvm.org/D55226

Files:
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  test/Analysis/security-syntax-checks.m


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -177,6 +177,11 @@
   strcpy(x, "abcd");
 }
 
+void test_strcpy_safe_2() {
+  struct {char s1[100];} s;
+  strcpy(s.s1, "hello");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -651,8 +651,8 @@
 
   const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
  *Source = CE->getArg(1)->IgnoreImpCasts();
-  if (const auto *DeclRef = dyn_cast(Target))
-if (const auto *Array = dyn_cast(DeclRef->getType())) {
+   if (isa(Target) || isa(Target))
+if (const auto *Array = dyn_cast(Target->getType())) {
   uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
   if (const auto *String = dyn_cast(Source)) {
 if (ArraySize >= String->getLength() + 1)


Index: test/Analysis/security-syntax-checks.m
===
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -177,6 +177,11 @@
   strcpy(x, "abcd");
 }
 
+void test_strcpy_safe_2() {
+  struct {char s1[100];} s;
+  strcpy(s.s1, "hello");
+}
+
 //===--===
 // strcat()
 //===--===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -651,8 +651,8 @@
 
   const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
  *Source = CE->getArg(1)->IgnoreImpCasts();
-  if (const auto *DeclRef = dyn_cast(Target))
-if (const auto *Array = dyn_cast(DeclRef->getType())) {
+	if (isa(Target) || isa(Target))
+if (const auto *Array = dyn_cast(Target->getType())) {
   uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
   if (const auto *String = dyn_cast(Source)) {
 if (ArraySize >= String->getLength() + 1)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348156 - Adding tests for -ast-dump; NFC.

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Mon Dec  3 10:00:31 2018
New Revision: 348156

URL: http://llvm.org/viewvc/llvm-project?rev=348156=rev
Log:
Adding tests for -ast-dump; NFC.

This adds tests for struct and union declarations in C++.

Added:
cfe/trunk/test/AST/ast-dump-records.cpp

Added: cfe/trunk/test/AST/ast-dump-records.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-records.cpp?rev=348156=auto
==
--- cfe/trunk/test/AST/ast-dump-records.cpp (added)
+++ cfe/trunk/test/AST/ast-dump-records.cpp Mon Dec  3 10:00:31 2018
@@ -0,0 +1,239 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++17 -ast-dump %s | 
FileCheck -strict-whitespace %s
+
+struct A;
+// CHECK: CXXRecordDecl 0x{{[^ ]*}} <{{.*}}:1, col:8> col:8 struct A
+
+struct B;
+// CHECK: CXXRecordDecl 0x{{[^ ]*}}  col:8 
referenced struct B
+
+struct A {
+  // CHECK: CXXRecordDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}}  line:[[@LINE-1]]:8 struct A definition
+  // CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+  // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
+  // CHECK-NEXT: CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
+  // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
+  // CHECK-NEXT: CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
+  // CHECK-NEXT: MoveAssignment exists simple trivial needs_implicit
+  // CHECK-NEXT: Destructor simple irrelevant trivial needs_implicit
+
+  // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  col:8 implicit 
struct A
+  int a;
+  // CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:7 a 
'int'
+  int b, c;
+  // CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:7 b 
'int'
+  // CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:10 c 'int'
+  int d : 12;
+  // CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:7 d 
'int'
+  // CHECK-NEXT: ConstantExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 12
+  int : 0;
+  // CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:7 'int'
+  // CHECK-NEXT: ConstantExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 0
+  int e : 10;
+  // CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:7 e 
'int'
+  // CHECK-NEXT: ConstantExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 10
+  B *f;
+  // CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:6 f 'B 
*'
+};
+
+struct C {
+  // CHECK: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:8 struct C definition
+  // CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal has_variant_members
+  // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
+  // CHECK-NEXT: CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
+  // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
+  // CHECK-NEXT: CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
+  // CHECK-NEXT: MoveAssignment exists simple trivial needs_implicit
+  // CHECK-NEXT: Destructor simple irrelevant trivial needs_implicit
+
+  // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  col:8 implicit 
struct C
+  struct {
+// CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
+// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
+// CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
+// CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
+// CHECK-NEXT: CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
+// CHECK-NEXT: MoveAssignment exists simple trivial needs_implicit
+// CHECK-NEXT: Destructor simple irrelevant trivial needs_implicit
+int a;
+// CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:9 a 
'int'
+  } b;
+  // CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:5 b 'struct (anonymous struct at 
{{.*}}:[[@LINE-12]]:3)':'C::(anonymous struct at {{.*}}:[[@LINE-12]]:3)'
+
+  union {
+// CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
+// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
+// CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
+// CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
+// CHECK-NEXT: CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
+// CHECK-NEXT: MoveAssignment exists simple trivial needs_implicit
+// CHECK-NEXT: Destructor simple irrelevant trivial needs_implicit
+int c;
+// CHECK-NEXT: FieldDecl 0x{{[^ ]*}}  col:9 c 
'int'
+float d;
+

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

from my side no objections, mailing list did not react AFAIK (just in case your 
waiting for me until you recommit).


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

https://reviews.llvm.org/D55006



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

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348154: Avoid emitting redundant or unusable directories in 
DIFile metadata entries. (authored by adrian, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55085?vs=176152=176422#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55085

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CodeGenAction.cpp
  test/CodeGen/debug-info-abspath.c
  test/CodeGen/debug-prefix-map.c
  test/Modules/module-debuginfo-prefix.m

Index: test/Modules/module-debuginfo-prefix.m
===
--- test/Modules/module-debuginfo-prefix.m
+++ test/Modules/module-debuginfo-prefix.m
@@ -20,4 +20,4 @@
 @import DebugObjC;
 #endif
 
-// CHECK: !DIFile({{.*}}"/OVERRIDE/DebugObjC.h"
+// CHECK: !DIFile(filename: "/OVERRIDE/DebugObjC.h", directory: "")
Index: test/CodeGen/debug-info-abspath.c
===
--- test/CodeGen/debug-info-abspath.c
+++ test/CodeGen/debug-info-abspath.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -triple %itanium_abi_triple \
+// RUN:   %s -emit-llvm -o - | FileCheck %s
+
+// RUN: cp %s %t.c
+// RUN: %clang_cc1 -debug-info-kind=limited -triple %itanium_abi_triple \
+// RUN:   %t.c -emit-llvm -o - | FileCheck %s --check-prefix=INTREE
+void foo() {}
+
+// Since %s is an absolute path, directory should be a nonempty
+// prefix, but the CodeGen part should be part of the filename.
+
+// CHECK: DIFile(filename: "{{.*}}CodeGen{{.*}}debug-info-abspath.c"
+// CHECK-SAME:   directory: "{{.+}}")
+
+// INTREE: DIFile({{.*}}directory: "{{.+}}CodeGen{{.*}}")
Index: test/CodeGen/debug-prefix-map.c
===
--- test/CodeGen/debug-prefix-map.c
+++ test/CodeGen/debug-prefix-map.c
@@ -17,18 +17,22 @@
 }
 
 // CHECK-NO-MAIN-FILE-NAME: !DIFile(filename: "/var/empty{{/|\\5C}}"
-// CHECK-NO-MAIN-FILE-NAME: !DIFile(filename: "/var/empty{{[/\\]}}{{.*}}"
-// CHECK-NO-MAIN-FILE-NAME: !DIFile(filename: "/var/empty{{[/\\]}}Inputs/stdio.h"
+// CHECK-NO-MAIN-FILE-NAME: !DIFile(filename: "/var/empty{{[/\\]}}{{.*}}",
+// CHECK-NO-MAIN-FILE-NAME-SAME:directory: "")
+// CHECK-NO-MAIN-FILE-NAME: !DIFile(filename: "/var/empty{{[/\\]}}Inputs/stdio.h",
+// CHECK-NO-MAIN-FILE-NAME-SAME:directory: "")
 // CHECK-NO-MAIN-FILE-NAME-NOT: !DIFile(filename:
 
 // CHECK-EVIL: !DIFile(filename: "/var=empty{{[/\\]}}{{.*}}"
-// CHECK-EVIL: !DIFile(filename: "/var=empty{{[/\\]}}Inputs/stdio.h"
+// CHECK-EVIL: !DIFile(filename: "/var=empty{{[/\\]}}{{.*}}Inputs/stdio.h",
+// CHECK-EVIL-SAME:directory: "")
 // CHECK-EVIL-NOT: !DIFile(filename:
 
 // CHECK: !DIFile(filename: "/var/empty{{[/\\]}}{{.*}}"
-// CHECK: !DIFile(filename: "/var/empty{{[/\\]}}Inputs/stdio.h"
+// CHECK: !DIFile(filename: "/var/empty{{[/\\]}}{{.*}}Inputs/stdio.h",
+// CHECK-SAME:directory: "")
 // CHECK-NOT: !DIFile(filename:
 
-// CHECK-COMPILATION-DIR: !DIFile(filename: "/var/empty{{[/\\]}}{{.*}}", directory: "/var/empty")
-// CHECK-COMPILATION-DIR: !DIFile(filename: "/var/empty{{[/\\]}}Inputs/stdio.h", directory: "/var/empty")
+// CHECK-COMPILATION-DIR: !DIFile(filename: "{{.*}}", directory: "/var/empty")
+// CHECK-COMPILATION-DIR: !DIFile(filename: "Inputs/stdio.h", directory: "/var/empty")
 // CHECK-COMPILATION-DIR-NOT: !DIFile(filename:
Index: lib/CodeGen/CodeGenAction.cpp
===
--- lib/CodeGen/CodeGenAction.cpp
+++ lib/CodeGen/CodeGenAction.cpp
@@ -549,12 +549,16 @@
   SourceLocation DILoc;
 
   if (D.isLocationAvailable()) {
-D.getLocation(, , );
-const FileEntry *FE = FileMgr.getFile(Filename);
-if (FE && Line > 0) {
-  // If -gcolumn-info was not used, Column will be 0. This upsets the
-  // source manager, so pass 1 if Column is not set.
-  DILoc = SourceMgr.translateFileLineCol(FE, Line, Column ? Column : 1);
+D.getLocation(Filename, Line, Column);
+if (Line > 0) {
+  const FileEntry *FE = FileMgr.getFile(Filename);
+  if (!FE)
+FE = FileMgr.getFile(D.getAbsolutePath());
+  if (FE) {
+// If -gcolumn-info was not used, Column will be 0. This upsets the
+// source manager, so pass 1 if Column is not set.
+DILoc = SourceMgr.translateFileLineCol(FE, Line, Column ? Column : 1);
+  }
 }
 BadDebugInfo = DILoc.isInvalid();
   }
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -181,8 +181,7 @@
   SourceManager  = CGM.getContext().getSourceManager();
   auto *Scope = cast(LexicalBlockStack.back());
   PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
-
-  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
+  if (PCLoc.isInvalid() 

r348154 - Avoid emitting redundant or unusable directories in DIFile metadata entries.

[Adrian Prantl via cfe-commits]

Author: adrian
Date: Mon Dec  3 09:55:27 2018
New Revision: 348154

URL: http://llvm.org/viewvc/llvm-project?rev=348154=rev
Log:
Avoid emitting redundant or unusable directories in DIFile metadata entries.

As discussed on llvm-dev recently, Clang currently emits redundant
directories in DIFile entries, such as

  .file  1 "/Volumes/Data/llvm" 
"/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"

This patch looks at any common prefix between the compilation
directory and the (absolute) file path and strips the redundant
part. More importantly it leaves the compilation directory empty if
the two paths have no common prefix.

After this patch the above entry is (assuming a compilation dir of 
"/Volumes/Data/llvm/_build"):

  .file 1 "/Volumes/Data/llvm" "tools/clang/test/CodeGen/debug-info-abspath.c"

When building the FileCheck binary with debug info, this patch makes
the build artifacts ~1kb smaller.

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

Added:
cfe/trunk/test/CodeGen/debug-info-abspath.c
Modified:
cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
cfe/trunk/lib/CodeGen/CodeGenAction.cpp
cfe/trunk/test/CodeGen/debug-prefix-map.c
cfe/trunk/test/Modules/module-debuginfo-prefix.m

Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348154=348153=348154=diff
==
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Dec  3 09:55:27 2018
@@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
   SourceManager  = CGM.getContext().getSourceManager();
   auto *Scope = cast(LexicalBlockStack.back());
   PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
-
-  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
+  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
 return;
 
   if (auto *LBF = dyn_cast(Scope)) {
@@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
   SourceManager  = CGM.getContext().getSourceManager();
   PresumedLoc PLoc = SM.getPresumedLoc(Loc);
 
-  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
+  StringRef FileName = PLoc.getFilename();
+  if (PLoc.isInvalid() || FileName.empty())
 // If the location is not valid then use main input file.
 return getOrCreateMainFile();
 
   // Cache the results.
-  const char *fname = PLoc.getFilename();
-  auto It = DIFileCache.find(fname);
+  auto It = DIFileCache.find(FileName.data());
 
   if (It != DIFileCache.end()) {
 // Verify that the information still exists.
@@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
   if (CSKind)
 CSInfo.emplace(*CSKind, Checksum);
 
-  llvm::DIFile *F = DBuilder.createFile(
-  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()), 
CSInfo,
-  getSource(SM, SM.getFileID(Loc)));
+  StringRef Dir;
+  StringRef File;
+  std::string RemappedFile = remapDIPath(FileName);
+  std::string CurDir = remapDIPath(getCurrentDirname());
+  SmallString<128> DirBuf;
+  SmallString<128> FileBuf;
+  if (llvm::sys::path::is_absolute(RemappedFile)) {
+// Strip the common prefix (if it is more than just "/") from current
+// directory and FileName for a more space-efficient encoding.
+auto FileIt = llvm::sys::path::begin(RemappedFile);
+auto FileE = llvm::sys::path::end(RemappedFile);
+auto CurDirIt = llvm::sys::path::begin(CurDir);
+auto CurDirE = llvm::sys::path::end(CurDir);
+for (; CurDirIt != CurDirE && *CurDirIt == *FileIt; ++CurDirIt, ++FileIt)
+  llvm::sys::path::append(DirBuf, *CurDirIt);
+if (std::distance(llvm::sys::path::begin(CurDir), CurDirIt) == 1) {
+  // The common prefix only the root; stripping it would cause
+  // LLVM diagnostic locations to be more confusing.
+  Dir = {};
+  File = RemappedFile;
+} else {
+  for (; FileIt != FileE; ++FileIt)
+llvm::sys::path::append(FileBuf, *FileIt);
+  Dir = DirBuf;
+  File = FileBuf;
+}
+  } else {
+Dir = CurDir;
+File = RemappedFile;
+  }
+  llvm::DIFile *F =
+  DBuilder.createFile(File, Dir, CSInfo,
+  getSource(SM, SM.getFileID(Loc)));
 
-  DIFileCache[fname].reset(F);
+  DIFileCache[FileName.data()].reset(F);
   return F;
 }
 

Modified: cfe/trunk/lib/CodeGen/CodeGenAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenAction.cpp?rev=348154=348153=348154=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenAction.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenAction.cpp Mon Dec  3 09:55:27 2018
@@ -549,12 +549,16 @@ const FullSourceLoc BackendConsumer::get
   SourceLocation DILoc;
 
   if (D.isLocationAvailable()) {
-D.getLocation(, , );
-const FileEntry *FE = FileMgr.getFile(Filename);
-if (FE && Line > 0) {
-   

[PATCH] D54657: [clang] Do not dump compilation-database entries for -E.

[Tom Roeder via Phabricator via cfe-commits]

tmroeder abandoned this revision.
tmroeder added a comment.

I found a better way to do this in the Linux kernel build.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54657



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

[PATCH] D32950: Support C++1z features in `__has_extension`

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

I don't mind picking it up again, but it won't be until next week.


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

https://reviews.llvm.org/D32950



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

[PATCH] D55134: [CTU] Add triple/lang mismatch handling

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be 
> an issue there?

I really don't know. We never tried it, our focus is on C and C++ for now. 
Unfortunately, there is nobody with ObjC/C++ knowledge and (more importantly) 
with interest to try CTU and report any errors.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55134



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

[PATCH] D55134: [CTU] Add triple/lang mismatch handling

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thanks for the review! I have updated the patch according to your comments.




Comment at: lib/CrossTU/CrossTranslationUnit.cpp:31
 
+namespace llvm {
+// Same as Triple's equality operator, but we check a field only if that is

a_sidorin wrote:
> Why don't we place it into the anon namespace just below?
Ok, I moved it into the unnamed namespace below.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:35
+bool hasEqualKnownFields(const Triple , const Triple ) {
+  return ((Lhs.getArch() != Triple::UnknownArch &&
+   Rhs.getArch() != Triple::UnknownArch)

a_sidorin wrote:
> This has to be split, probably with early returns. Example:
> ```if (Lhs.getArch() != Triple::UnknownArch && Rhs.getArch() != 
> Triple::UnknownArch &&
>   Lhs.getArch() != Rhs.getArch()
>   return false;
> ...```
Ok, I split that up with early returns.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:59
+}
+}
+

a_sidorin wrote:
> Szelethus wrote:
> > `// end of namespace llvm`
> `// end namespace llvm` is much more common.
Moved to the unnamed namespace.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55134



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

[PATCH] D55134: [CTU] Add triple/lang mismatch handling

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 176417.
martong marked 13 inline comments as done.
martong added a comment.

- Address review comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D55134

Files:
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/ctu-different-triples.cpp
  test/Analysis/ctu-unknown-parts-in-triples.cpp

Index: test/Analysis/ctu-unknown-parts-in-triples.cpp
===
--- /dev/null
+++ test/Analysis/ctu-unknown-parts-in-triples.cpp
@@ -0,0 +1,16 @@
+// We do not expect any error when one part of the triple is unknown, but other
+// known parts are equal.
+
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %t/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp
+// RUN: cp %S/Inputs/externalFnMap.txt %t/ctudir/externalFnMap.txt
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%t/ctudir -verify %s
+
+// expected-no-diagnostics
+
+int f(int);
+
+int main() {
+  return f(5);
+}
Index: test/Analysis/ctu-different-triples.cpp
===
--- /dev/null
+++ test/Analysis/ctu-different-triples.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %t/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp
+// RUN: cp %S/Inputs/externalFnMap.txt %t/ctudir/externalFnMap.txt
+// RUN: %clang_cc1 -triple powerpc64-montavista-linux-gnu -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%t/ctudir -verify %s
+
+// We expect an error in this file, but without a location.
+// expected-error-re@./ctu-different-triples.cpp:*{{imported AST from {{.*}} had been generated for a different target, current: powerpc64-montavista-linux-gnu, imported: x86_64-pc-linux-gnu}}
+
+int f(int);
+
+int main() {
+  return f(5);
+}
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -32,6 +32,36 @@
 namespace cross_tu {
 
 namespace {
+
+// Same as Triple's equality operator, but we check a field only if that is
+// known in both instances.
+bool hasEqualKnownFields(const llvm::Triple , const llvm::Triple ) {
+  using llvm::Triple;
+  if (Lhs.getArch() != Triple::UnknownArch &&
+  Rhs.getArch() != Triple::UnknownArch && Lhs.getArch() != Rhs.getArch())
+return false;
+  if (Lhs.getSubArch() != Triple::NoSubArch &&
+  Rhs.getSubArch() != Triple::NoSubArch &&
+  Lhs.getSubArch() != Rhs.getSubArch())
+return false;
+  if (Lhs.getVendor() != Triple::UnknownVendor &&
+  Rhs.getVendor() != Triple::UnknownVendor &&
+  Lhs.getVendor() != Rhs.getVendor())
+return false;
+  if (!Lhs.isOSUnknown() && !Rhs.isOSUnknown() &&
+  Lhs.getOS() != Rhs.getOS())
+return false;
+  if (Lhs.getEnvironment() != Triple::UnknownEnvironment &&
+  Rhs.getEnvironment() != Triple::UnknownEnvironment &&
+  Lhs.getEnvironment() != Rhs.getEnvironment())
+return false;
+  if (Lhs.getObjectFormat() != Triple::UnknownObjectFormat &&
+  Rhs.getObjectFormat() != Triple::UnknownObjectFormat &&
+  Lhs.getObjectFormat() != Rhs.getObjectFormat())
+return false;
+  return true;
+}
+
 // FIXME: This class is will be removed after the transition to llvm::Error.
 class IndexErrorCategory : public std::error_category {
 public:
@@ -55,6 +85,10 @@
   return "Failed to load external AST source.";
 case index_error_code::failed_to_generate_usr:
   return "Failed to generate USR.";
+case index_error_code::triple_mismatch:
+  return "Triple mismatch";
+case index_error_code::lang_mismatch:
+  return "Language mismatch";
 }
 llvm_unreachable("Unrecognized index_error_code.");
   }
@@ -166,6 +200,30 @@
   assert(>getFileManager() ==
  >getASTContext().getSourceManager().getFileManager());
 
+  const auto  = Context.getTargetInfo().getTriple();
+  const auto  = Unit->getASTContext().getTargetInfo().getTriple();
+  // The imported AST had been generated for a different target.
+  // Some parts of the triple in the loaded ASTContext can be unknown while the
+  // very same parts in the target ASTContext are known. Thus we check for the
+  // known parts only.
+  if (!hasEqualKnownFields(TripleTo, TripleFrom)) {
+// TODO: Pass the SourceLocation of the CallExpression for more precise
+// diagnostics.
+

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

[Davide Italiano via Phabricator via cfe-commits]

davide accepted this revision.
davide added a comment.

LGTM, sorry. for the delay.


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

https://reviews.llvm.org/D55085



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

[PATCH] D55188: Extract TextChildDumper class

[Stephen Kelly via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/ASTDumperUtils.h:96
+
+struct TextChildDumper {
+  raw_ostream 

steveire wrote:
> aaron.ballman wrote:
> > I'm not sold on the name for this class. It's a bit too generic to 
> > understand what it does. How about `ASTDumpLayoutFormatter` (and 
> > `ASTDumpNodeFormatter` for the node dumper)?
> I'll rename the class to `ASTTextTreeStructure`.
Or rather, `TextTreeStructure`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55188



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

[PATCH] D55057: [Headers] Make max_align_t match GCC's implementation.

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added inline comments.



Comment at: lib/Headers/__stddef_max_align_t.h:40
   __attribute__((__aligned__(__alignof__(long double;
+#ifdef __i386__
+  __float128 __clang_max_align_nonce3

jyknight wrote:
> uweigand wrote:
> > jyknight wrote:
> > > Can you fix clang to consistently define `__SIZEOF_FLOAT128__` in 
> > > InitPreprocessor alongside the rest of the SIZEOF macros?
> > > 
> > > And then use that to determine whether to add float128 to the union? This 
> > > change, as is, will break on any target which is i386 but doesn't define 
> > > __float128, e.g. i386-unknown-unknown.
> > > 
> > > The only additional target which will be modified with that (that is: the 
> > > only other target which has a float128 type, but for which max_align 
> > > isn't already 16) is systemz-*-linux.
> > > 
> > > But I think that's actually indicating a pre-existing bug in the SystemZ 
> > > config -- it's configured for a 16-byte long double, with 8-byte 
> > > alignment, but the ABI doc seems to call for 16-byte alignment. +Ulrich 
> > > for comment on that.
> > That's a bug in the ABI doc which we'll fix once we get around to releasing 
> > an updated version.
> > 
> > long double on SystemZ must be 8-byte aligned, which is the maximum 
> > alignment of all standard types on Z, and this was chosen because 
> > historically the ABI only guarantees an 8-byte stack alignment, so 16-byte 
> > aligned standard types would be awkward.
> Then perhaps it's a bug that `__alignof__(__float128)` returns 16 with 
> `-target systemz-linux`?
Huh, really __float128 should not be supported at all on SystemZ.  It's not 
supported with GCC either (GCC never provides __float128 on targets where long 
double is already IEEE-128).  (GCC does support the new _Float128 on SystemZ, 
but that's not yet supported by clang anywhere.)

If it were supported, I agree it should be an alias for long double, and also 
have an alignof of 8.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55057



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

[PATCH] D55188: Extract TextChildDumper class

[Stephen Kelly via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/ASTDumperUtils.h:22
+
+namespace ast_dumper {
+// Colors used for various parts of the AST dump

aaron.ballman wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > I'm not certain this namespace is useful, especially when it gets 
> > > imported at TU scope in ASTDumper.cpp.
> > > it gets imported at TU scope in ASTDumper.cpp
> > 
> > Today that's the only place it is used. In the future that won't be true.
> Then we can add the namespace in the future when we find a situation where 
> we're worried about collisions? As it stands, this only provides the illusion 
> of safety. If you really want to keep it, please pull the global using 
> declaration.
There is no need for such churn as adding the namespace later.

The `using` I added is beside two other `using`s in a cpp file. There is 
nothing 'global' about it.



Comment at: include/clang/AST/ASTDumperUtils.h:96
+
+struct TextChildDumper {
+  raw_ostream 

aaron.ballman wrote:
> I'm not sold on the name for this class. It's a bit too generic to understand 
> what it does. How about `ASTDumpLayoutFormatter` (and `ASTDumpNodeFormatter` 
> for the node dumper)?
I'll rename the class to `ASTTextTreeStructure`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55188



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54757#1316899 , @donat.nagy wrote:

> I applied this check to the llvm + clang codebase, and I was surprised to see 
> that it produced about 600 results (for comparison, the clang-tidy checks 
> which are enabled by default produce approximately 6000 results together). I 
> didn't go through the whole list, but after examining a few dozen random 
> reports it seems that most of these are true positives: relatively short 
> branches (usually one or two lines of code) are repeated for no obvious 
> reason. I would guess that most of these fall into the "correct but too 
> verbose" case, because otherwise the tests would have caught the problem, but 
> I didn't try to understand their context.
>
> I have seen a few false positives related to preprocessor trickery: when an 
> `.inc` file is included to create a huge switch, sometimes it will become a 
> huge switch with lots of identical branches. There were also some situations 
> where the check reported identical branches which are annotated with 
> different comments; I don't know if this should be considered a false 
> positive.
>
> If this is acceptable, then I would be grateful if someone would commit this 
> patch for me as I don't have commit rights.


Thank you!
Are the cases with `.inc` files easily silenceable with `// NOLINT` comments? 
In general code-generation through macros is not easily handled with 
clang-tidy, so all checks have the problem (e.g. GoogleTest is extremly chatty 
with everything), but would be interesting to know :)

All other cases sound like "a human should look and evaluate" and that's ok for 
clang-tidy as it doesn't claim finding ONLY bugs, but all kind of issues.

Thank you for the patch, if there are no objections by other reviewers I will 
commit tomorrow( ~24 hours from now)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D55062: [clangd] Partition include graph on auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 176415.
kadircet marked 3 inline comments as done.
kadircet added a comment.

- Addresss comments
- Make sure there are no uninitialized values in IncludeGraphNode.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55062

Files:
  clangd/Headers.h
  clangd/index/Background.cpp
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -24,6 +24,11 @@
 RefsAre(std::vector> Matchers) {
   return ElementsAre(testing::Pair(_, UnorderedElementsAreArray(Matchers)));
 }
+// URI cannot be empty since it references keys in the IncludeGraph.
+MATCHER(EmptyIncludeNode, "") {
+  return !arg.IsTU && !arg.URI.empty() && arg.Digest == FileDigest{0} &&
+ arg.DirectIncludes.empty();
+}
 
 class MemoryShardStorage : public BackgroundIndexStorage {
   mutable std::mutex StorageMu;
@@ -93,7 +98,7 @@
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
-  CDB.setCompileCommand(testPath("root"), Cmd);
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   EXPECT_THAT(
@@ -103,7 +108,7 @@
 
   Cmd.Filename = testPath("root/B.cc");
   Cmd.CommandLine = {"clang++", Cmd.Filename};
-  CDB.setCompileCommand(testPath("root"), Cmd);
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   // B_CC is dropped as we don't collect symbols from A.h in this compilation.
@@ -143,7 +148,7 @@
 OverlayCDB CDB(/*Base=*/nullptr);
 BackgroundIndex Idx(Context::empty(), "", FS, CDB,
 [&](llvm::StringRef) { return  });
-CDB.setCompileCommand(testPath("root"), Cmd);
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 0U);
@@ -165,5 +170,52 @@
   EXPECT_THAT(*ShardSource->Refs, RefsAre({FileURI("unittest:///root/A.cc")}));
 }
 
+TEST_F(BackgroundIndexTest, DirectIncludesTest) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/B.h")] = "";
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  #include "B.h"
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  std::string A_CC = "#include \"A.h\"\nvoid g() { (void)common; }";
+  FS.Files[testPath("root/A.cc")] = A_CC;
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return  });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_TRUE(ShardSource->Sources);
+  EXPECT_EQ(ShardSource->Sources->size(), 2U); // A.cc, A.h
+  EXPECT_THAT(
+  ShardSource->Sources->lookup("unittest:///root/A.cc").DirectIncludes,
+  UnorderedElementsAre("unittest:///root/A.h"));
+  EXPECT_THAT(ShardSource->Sources->lookup("unittest:///root/A.h"),
+  EmptyIncludeNode());
+
+  auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
+  EXPECT_TRUE(ShardHeader->Sources);
+  EXPECT_EQ(ShardHeader->Sources->size(), 2U); // A.h B.h
+  EXPECT_THAT(
+  ShardHeader->Sources->lookup("unittest:///root/A.h").DirectIncludes,
+  UnorderedElementsAre("unittest:///root/B.h"));
+  EXPECT_THAT(ShardHeader->Sources->lookup("unittest:///root/B.h"),
+  EmptyIncludeNode());
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -35,6 +35,58 @@
 using namespace llvm;
 namespace clang {
 namespace clangd {
+namespace {
+// Resolves URI to file paths with cache.
+class URIToFileCache {
+public:
+  URIToFileCache(llvm::StringRef HintPath) : HintPath(HintPath) {}
+
+  llvm::StringRef resolve(llvm::StringRef FileURI) {
+auto I = URIToPathCache.try_emplace(FileURI);
+if (I.second) {
+  auto U = URI::parse(FileURI);
+  if (!U) {
+elog("Failed to parse URI {0}: {1}", FileURI, U.takeError());
+assert(false && "Failed to parse URI");
+return "";
+  }
+  auto Path = URI::resolve(*U, HintPath);
+  if (!Path) {
+elog("Failed to resolve URI {0}: {1}", FileURI, Path.takeError());
+assert(false && "Failed to resolve URI");
+return "";
+  }
+  

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 2 inline comments as done.
donat.nagy added a comment.

I applied this check to the llvm + clang codebase, and I was surprised to see 
that it produced about 600 results (for comparison, the clang-tidy checks which 
are enabled by default produce approximately 6000 results together). I didn't 
go through the whole list, but after examining a few dozen random reports it 
seems that most of these are true positives: relatively short branches (usually 
one or two lines of code) are repeated for no obvious reason. I would guess 
that most of these fall into the "correct but too verbose" case, because 
otherwise the tests would have caught the problem, but I didn't try to 
understand their context.

I have seen a few false positives related to preprocessor trickery: when an 
`.inc` file is included to create a huge switch, sometimes it will become a 
huge switch with lots of identical branches. There were also some situations 
where the check found identical branches which are annotated with different 
comments; I don't know if this should be considered a false positive.

If this is acceptable, then I would be grateful if someone would commit this 
patch for me as I don't have commit rights.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

r348150 - [Serialization][NFC] Remove pointless "+ 0" in ASTReader

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Mon Dec  3 08:17:45 2018
New Revision: 348150

URL: http://llvm.org/viewvc/llvm-project?rev=348150=rev
Log:
[Serialization][NFC] Remove pointless "+ 0" in ASTReader

Remove the pointless "+ 0" which I added for some reason when
modifying these statement/expression classes since it looks
like this is a typo. Following the suggestion of aaron.ballman
in D54902. NFC.


Modified:
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp

Modified: cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderStmt.cpp?rev=348150=348149=348150=diff
==
--- cfe/trunk/lib/Serialization/ASTReaderStmt.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReaderStmt.cpp Mon Dec  3 08:17:45 2018
@@ -2347,14 +2347,14 @@ Stmt *ASTReader::ReadStmtFromStream(Modu
 case STMT_SWITCH:
   S = SwitchStmt::CreateEmpty(
   Context,
-  /* HasInit=*/Record[ASTStmtReader::NumStmtFields + 0],
+  /* HasInit=*/Record[ASTStmtReader::NumStmtFields],
   /* HasVar=*/Record[ASTStmtReader::NumStmtFields + 1]);
   break;
 
 case STMT_WHILE:
   S = WhileStmt::CreateEmpty(
   Context,
-  /* HasVar=*/Record[ASTStmtReader::NumStmtFields + 0]);
+  /* HasVar=*/Record[ASTStmtReader::NumStmtFields]);
   break;
 
 case STMT_DO:
@@ -2438,7 +2438,7 @@ Stmt *ASTReader::ReadStmtFromStream(Modu
 case EXPR_STRING_LITERAL:
   S = StringLiteral::CreateEmpty(
   Context,
-  /* NumConcatenated=*/Record[ASTStmtReader::NumExprFields + 0],
+  /* NumConcatenated=*/Record[ASTStmtReader::NumExprFields],
   /* Length=*/Record[ASTStmtReader::NumExprFields + 1],
   /* CharByteWidth=*/Record[ASTStmtReader::NumExprFields + 2]);
   break;
@@ -2454,7 +2454,7 @@ Stmt *ASTReader::ReadStmtFromStream(Modu
 case EXPR_PAREN_LIST:
   S = ParenListExpr::CreateEmpty(
   Context,
-  /* NumExprs=*/Record[ASTStmtReader::NumExprFields + 0]);
+  /* NumExprs=*/Record[ASTStmtReader::NumExprFields]);
   break;
 
 case EXPR_UNARY_OPERATOR:
@@ -2481,7 +2481,7 @@ Stmt *ASTReader::ReadStmtFromStream(Modu
 
 case EXPR_CALL:
   S = new (Context) CallExpr(
-  Context, /* NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Context, /* NumArgs=*/Record[ASTStmtReader::NumExprFields],
   Empty);
   break;
 
@@ -3073,13 +3073,13 @@ Stmt *ASTReader::ReadStmtFromStream(Modu
 
 case EXPR_CXX_OPERATOR_CALL:
   S = new (Context) CXXOperatorCallExpr(
-  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields],
   Empty);
   break;
 
 case EXPR_CXX_MEMBER_CALL:
   S = new (Context) CXXMemberCallExpr(
-  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0],
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields],
   Empty);
   break;
 
@@ -3121,7 +3121,7 @@ Stmt *ASTReader::ReadStmtFromStream(Modu
 
 case EXPR_USER_DEFINED_LITERAL:
   S = new (Context) UserDefinedLiteral(
-  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0], 
Empty);
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields], Empty);
   break;
 
 case EXPR_CXX_STD_INITIALIZER_LIST:
@@ -3292,7 +3292,7 @@ Stmt *ASTReader::ReadStmtFromStream(Modu
 
 case EXPR_CUDA_KERNEL_CALL:
   S = new (Context) CUDAKernelCallExpr(
-  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields + 0], 
Empty);
+  Context, /*NumArgs=*/Record[ASTStmtReader::NumExprFields], Empty);
   break;
 
 case EXPR_ASTYPE:


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

[PATCH] D53768: Add VerboseOutputStream to CompilerInstance

[Scott Linder via Phabricator via cfe-commits]

scott.linder added a comment.

Ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D53768



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

[PATCH] D55225: [AST] Assert that no type class is polymorphic

[Bruno Ricci via Phabricator via cfe-commits]

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

Add a `static_assert` checking that no type class is polymorphic.
People should use LLVM style RTTI instead.


Repository:
  rC Clang

https://reviews.llvm.org/D55225

Files:
  lib/AST/Type.cpp


Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -291,6 +291,14 @@
   return Context.getQualifiedType(desugar, split.Quals);
 }
 
+// Check that no type class is polymorphic. LLVM style RTTI should be used
+// instead. If absolutely needed an exception can still be added here by
+// defining the appropriate macro (but please don't do this).
+#define TYPE(CLASS, BASE) \
+  static_assert(!std::is_polymorphic::value, \
+#CLASS "Type should not be polymorphic!");
+#include "clang/AST/TypeNodes.def"
+
 QualType Type::getLocallyUnqualifiedSingleStepDesugaredType() const {
   switch (getTypeClass()) {
 #define ABSTRACT_TYPE(Class, Parent)


Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -291,6 +291,14 @@
   return Context.getQualifiedType(desugar, split.Quals);
 }
 
+// Check that no type class is polymorphic. LLVM style RTTI should be used
+// instead. If absolutely needed an exception can still be added here by
+// defining the appropriate macro (but please don't do this).
+#define TYPE(CLASS, BASE) \
+  static_assert(!std::is_polymorphic::value, \
+#CLASS "Type should not be polymorphic!");
+#include "clang/AST/TypeNodes.def"
+
 QualType Type::getLocallyUnqualifiedSingleStepDesugaredType() const {
   switch (getTypeClass()) {
 #define ABSTRACT_TYPE(Class, Parent)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55212: Handle alloc_size attribute on function pointers

[Alexander Richardson via Phabricator via cfe-commits]

arichardson updated this revision to Diff 176413.
arichardson added a comment.

- address review comments
- add test that we can assign between function pointers with and without 
alloc_size attribute.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55212

Files:
  include/clang/Basic/Attr.td
  lib/AST/ExprConstant.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/alloc-size-fnptr.c
  test/CodeGen/alloc-size.c
  test/Sema/alloc-size.c

Index: test/Sema/alloc-size.c
===
--- test/Sema/alloc-size.c
+++ test/Sema/alloc-size.c
@@ -14,7 +14,7 @@
 
 int fail9(int a) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to return values that are pointers}}
 
-int fail10 __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}
+int fail10 __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to non-K functions}}
 
 void *fail11(void *a) __attribute__((alloc_size(1))); //expected-error{{'alloc_size' attribute argument may only refer to a function parameter of integer type}}
 
@@ -22,4 +22,41 @@
 void *fail12(int a) __attribute__((alloc_size(1, "abc"))); //expected-error{{'alloc_size' attribute requires parameter 2 to be an integer constant}}
 void *fail13(int a) __attribute__((alloc_size(1U<<31))); //expected-error{{integer constant expression evaluates to value 2147483648 that cannot be represented in a 32-bit signed integer type}}
 
-int (*PR31453)(int) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}
+void *(*PR31453)(int)__attribute__((alloc_size(1)));
+
+void *KR() __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to non-K functions}}
+
+// Applying alloc_size to function pointers should work:
+void *(__attribute__((alloc_size(1))) * func_ptr1)(int);
+void *(__attribute__((alloc_size(1, 2))) func_ptr2)(int, int);
+
+// TODO: according to GCC documentation the following should actually be the type
+// “pointer to pointer to alloc_size attributed function returning void*” and should
+// therefore be supported
+void *(__attribute__((alloc_size(1))) **ptr_to_func_ptr)(int); // expected-warning{{'alloc_size' attribute only applies to non-K functions}}
+// The following definitions apply the attribute to the pointer to the function pointer which should not be possible
+void *(*__attribute__((alloc_size(1))) * ptr_to_func_ptr2)(int); // expected-warning{{'alloc_size' attribute only applies to non-K functions}}
+void *(**__attribute__((alloc_size(1))) ptr_to_func_ptr2)(int);  // expected-warning{{'alloc_size' attribute only applies to non-K functions}}
+
+// It should also work for typedefs:
+typedef void *(__attribute__((alloc_size(1))) allocator_function_typdef)(int);
+typedef void *(__attribute__((alloc_size(1, 2))) * allocator_function_typdef2)(int, int);
+void *(__attribute__((alloc_size(1, 2))) * allocator_function_typdef3)(int, int);
+// This typedef applies the alloc_size to the pointer to the function pointer and should not be allowed
+void *(**__attribute__((alloc_size(1, 2))) * allocator_function_typdef4)(int, int); // expected-warning{{'alloc_size' attribute only applies to non-K functions}}
+
+
+
+// We should not be warning when assigning function pointers with and without the alloc size attribute
+// since it doesn't change the type of the function
+typedef void * (__attribute__((alloc_size(1))) * my_malloc_fn_pointer_type)(int);
+typedef void * (* my_other_malloc_fn_pointer_type)(int);
+void *fn(int i);
+__attribute__((alloc_size(1))) void *fn2(int i);
+
+int main() {
+  my_malloc_fn_pointer_type f = fn;
+  my_other_malloc_fn_pointer_type f2 = fn;
+  my_malloc_fn_pointer_type f3 = fn2;
+  my_other_malloc_fn_pointer_type f4 = fn2;
+}
Index: test/CodeGen/alloc-size.c
===
--- test/CodeGen/alloc-size.c
+++ test/CodeGen/alloc-size.c
@@ -350,3 +350,128 @@
   // CHECK: store i32 -1
   gi = __builtin_object_size(my_signed_calloc(-2, 1), 0);
 }
+
+void* (*malloc_function_pointer)(int) __attribute__((alloc_size(1)));
+void* (*calloc_function_pointer)(int, int) __attribute__((alloc_size(1, 2)));
+
+// CHECK-LABEL: @test_fn_pointer
+void test_fn_pointer() {
+  void *const vp = malloc_function_pointer(100);
+  // CHECK: store i32 100
+  gi = __builtin_object_size(vp, 0);
+  // CHECK: store i32 100
+  gi = __builtin_object_size(vp, 1);
+  // CHECK: store i32 100
+  gi = __builtin_object_size(vp, 2);
+  // CHECK: store i32 100
+  gi = __builtin_object_size(vp, 3);
+
+  void *const arr = calloc_function_pointer(100, 5);
+  // CHECK: store i32 500
+  gi = __builtin_object_size(arr, 0);
+  // CHECK: store i32 500
+  gi = __builtin_object_size(arr, 1);
+  // CHECK: store i32 500
+  gi = 

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Ulrich Weigand via Phabricator via cfe-commits]

uweigand added a comment.

In D53157#1309743 , @cameron.mcinally 
wrote:

> Digressing a bit, but has anyone given thought to how this implementation 
> will play out with libraries? When running with traps enabled, libraries must 
> be compiled for trap-safety. E.g. optimizations on a lib's code could 
> introduce a NaN or cause a trap that does not exist in the code itself.


In my reading of the standard text, there is no special provision for library 
code.  This means that in general, calling any library function while running 
with nondefault trap settings is undefined behavior, unless the library was 
itself compiled with FENV_ACCESS ON.  There does not even appear to be any 
requirement for the C standard library functions to have been compiled with 
FENV_ACCESS ON, as far as I can see ...


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

https://reviews.llvm.org/D53157



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

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Whenever a change happens on a CDB, load shards associated with that
CDB before issuing re-index actions.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55224

Files:
  clangd/index/Background.cpp
  clangd/index/Background.h
  test/clangd/background-index.test
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -7,6 +7,7 @@
 
 using testing::_;
 using testing::AllOf;
+using testing::Contains;
 using testing::Not;
 using testing::UnorderedElementsAre;
 
@@ -120,7 +121,7 @@
FileURI("unittest:///root/B.cc")}));
 }
 
-TEST_F(BackgroundIndexTest, ShardStorageWriteTest) {
+TEST_F(BackgroundIndexTest, ShardStorageTest) {
   MockFSProvider FS;
   FS.Files[testPath("root/A.h")] = R"cpp(
   void common();
@@ -149,6 +150,16 @@
   EXPECT_EQ(CacheHits, 0U);
   EXPECT_EQ(Storage.size(), 2U);
 
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return  });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+  EXPECT_EQ(Storage.size(), 2U);
+
   auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_THAT(
@@ -208,5 +219,73 @@
   UnorderedElementsAre("unittest:///root/B.h"));
 }
 
+TEST_F(BackgroundIndexTest, ShardStorageLoad) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return  });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+
+  // Change header.
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  class A_CCnew {};
+  )cpp";
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return  });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+
+  // Change source.
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }\nvoid f_b() {}";
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return  });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardSource->Symbols,
+  Contains(AllOf(Named("f_b"), Declared(), Defined(;
+}
+
 } // namespace clangd
 } // namespace clang
Index: test/clangd/background-index.test
===
--- test/clangd/background-index.test
+++ test/clangd/background-index.test
@@ -16,6 +16,5 @@
 # RUN: ls %t/.clangd-index/foo.cpp.*.idx
 
 # Test the index is read from disk: delete code and restart clangd.
-# FIXME: This test currently fails as we don't read the index yet.
 # RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | not FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -74,7 +74,6 @@
   // The indexing happens in a background 

[PATCH] D55212: Handle alloc_size attribute on function pointers

[Alexander Richardson via Phabricator via cfe-commits]

arichardson marked 5 inline comments as done.
arichardson added inline comments.



Comment at: include/clang/Basic/Attr.td:1072
 def AllocSize : InheritableAttr {
   let Spellings = [GCC<"alloc_size">];
+  let Subjects = SubjectList<[HasFunctionProto]>;

aaron.ballman wrote:
> Does GCC support writing `alloc_size` on function pointers?
Yes it does and it seems to be used by some projects. I first discovered this 
while compiling libxml2: 
https://github.com/GNOME/libxml2/blob/35e83488505d501864826125cfe6a7950d6cba78/include/libxml/xmlmemory.h#L66



Comment at: test/Sema/alloc-size.c:46
+// This typedef applies the alloc_size to the pointer to the function pointer 
and should not be allowed
+void *(**__attribute__((alloc_size(1, 2))) * allocator_function_typdef4)(int, 
int); // expected-warning{{'alloc_size' attribute only applies to non-K 
functions}}

aaron.ballman wrote:
> What should happen in these situations?
> ```
> typedef void * (__attribute__((alloc_size(1))) * 
> my_malloc_fn_pointer_type)(int);
> typedef void * (* my_other_malloc_fn_pointer_type)(int);
> 
> void *fn(int i);
> __attribute__((alloc_size(1))) void *fn2(int i);
> 
> int main() {
>   my_malloc_fn_pointer_type f = fn; // ???
>   my_other_malloc_fn_pointer_type f2 = fn2; // ???
> }
> ```
> Should this code do something special?
> ```
> typedef void * (__attribute__((alloc_size(1))) * 
> my_malloc_fn_pointer_type)(int);
> typedef void * (* my_other_malloc_fn_pointer_type)(int);
> 
> void overloaded(my_malloc_fn_pointer_type fn);
> void overloaded(my_other_malloc_fn_pointer_type fn);
> 
> void *fn(int i);
> __attribute__((alloc_size(1))) void *fn2(int i);
> 
> int main() {
>   overloaded(fn);
>   overloaded(fn2);
> }
> 
> ```
If I define two overloaded functions that only differ on the attribute GCC 
gives me the following error:
```
:14:6: error: redefinition of 'overloaded'

   14 | void overloaded(my_other_malloc_fn_pointer_type fn) {

  |  ^~

:11:6: note: previous definition of 'overloaded' was here

   11 | void overloaded(my_malloc_fn_pointer_type fn) {

  |  ^~
```
Assigning function pointers with and without the attribute seems to work just 
fine:
https://godbolt.org/z/-i5zUK


Repository:
  rC Clang

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

https://reviews.llvm.org/D55212



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

[PATCH] D54796: [clangd] C++ API for emitting file status

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for the delays. Not sure about emitting `Queued` on the main thread, see 
the corresponding comment. Otherwise looks very good.




Comment at: clangd/ClangdServer.h:49
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(PathRef File, const TUStatus ){};
 };

hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > Have we thought about the way we might expose statuses via the LSP?
> > > > The `TUStatus` seems a bit too clangd-specific (i.e. 
> > > > `BuildingPreamble`, `ReuseAST` is not something that makes sense in the 
> > > > protocol). Which might be fine on the `ClangdServer` layer, but it 
> > > > feels like we should generalize before sending it over via the LSP
> > > > The TUStatus seems a bit too clangd-specific (i.e. BuildingPreamble, 
> > > > ReuseAST is not something that makes sense in the protocol). 
> > > 
> > > Yes, this is by design. `TUStatus` presents the internal states of 
> > > clangd, and would not be exposed via LSP.
> > > 
> > > Some ways of exposing status via the LSP
> > > - define a separate structure e.g. `FileStatus` in LSP,  render 
> > > `TUStatus` to it, and sending it to clients (probably we might want to 
> > > add a new extension `textDocument/filestatus`)
> > > - reuse some existing LSP methods (`window/showMessage`, 
> > > `window/logMessage`), render `TUStatus` to one of these methods' params.
> > LG, I believe the `BuildingPreamble` status in particular might be useful 
> > on the C++ API level for us.
> > Not sure `showMessage` or `logMessage` is a good idea, the first one will 
> > definitely be annoying if it'll be popping up all the time. Having a new 
> > method in the LSP LG.
> `showMessage` seems work well via vim YCM, but it is annoying in vscode (as 
> it pops up diags) -- we need to customize this behavior in our own extensions 
> (showing messages in status bar). We need to revisit different ways. 
I'd err on the side of getting a new LSP method/extension that's supposed to 
show messages in the status bar.
It seems `showMessage` was specifically designed for user interactions and not 
status bar updates. But that's out of the scope of this patch, so definitely 
not blocking it.



Comment at: clangd/TUScheduler.cpp:268
+  /// Status of the TU.
+  TUStatus Status; /* GUARDED_BY(DiagMu) */
 };

hokein wrote:
> ilya-biryukov wrote:
> > Is `Status` actually ever read from multiple threads?
> > I assume it's only accessed on the worker thread, right? I think we can 
> > leave out the locks around it and put beside other fields only accessed by 
> > the worker thread, i.e. `DiagsWereReported`, etc.
> > 
> > PS Sorry go going back and forth, I didn't think about it in the previous 
> > review iteration.
> Unfortunately not, most statuses are emitted via worker thread, but we emit 
> `Queued` status in the main thread...
Hm, that sounds surprising.
What if the rebuild is in progress on the worker thread and we emit the queued 
status on the main thread at the same time? We might get weird sequences of 
statuses, right? 
E.g. `Queued → BuildingPreamble → [new update on the main thread] Queued → 
[processing request on a worker thread] RunningAction('XRefs')`

The `Queued` request status between `BuildingPreamble` and `RunningAction` 
looks **really** weird.
Maybe we could avoid that by setting emitting the `Queued` status on the worker 
thread whenever we're about to sleep on the debounce timeout?



Comment at: clangd/TUScheduler.cpp:636
+  if (Requests.empty())
+emitTUStatus({TUAction::Idle, /*Name*/ ""});
 }

hokein wrote:
> ilya-biryukov wrote:
> > Maybe do it outside the lock? The less dependencies between the locks we 
> > have, the better and this one seems unnecessary.
> Is it safe to read the `Requests` out of the lock here?
No, but we could store the value of `.empty()` in a local var under the lock. 
The code looks a bit weirder, but that's a small price to pay for less locks 
being held at the same time.



Comment at: clangd/TUScheduler.h:57
+  enum State {
+Unknown,
+Queued,   // The TU is pending in the thread task queue to be 
built.

hokein wrote:
> ilya-biryukov wrote:
> > Maybe remove this value and leave out the default constructor too?
> > That would ensure we never fill have `State` field with undefined value 
> > without having this enumerator.
> > And FWIW, putting **any** value as the default seems fine even if we want 
> > to keep the default ctor, all the users have to set the State anyway. 
> We need the default constructor... When ASTWorker is constructed, we don't 
> know the any state, I'm nervous about putting **any value** as default value 
> (it will hurt the code readability, future readers might be confused -- why 
> the status is `Queued` when constructing).
> 
> Adding 

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 176408.
donat.nagy added a comment.

Minor correction in documentation


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757

Files:
  clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tidy/bugprone/BranchCloneCheck.h
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-branch-clone.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-branch-clone.cpp

Index: test/clang-tidy/bugprone-branch-clone.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,1017 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int ) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic2(int in, int ) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+  }
+
+  out++;
+}
+
+void test_basic3(int in, int ) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic4(int in, int ) {
+  if (in > 77) {
+out--;
+  }
+  else {
+out++;
+  }
+}
+
+void test_basic5(int in, int ) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++;
+out++;
+  }
+}
+
+void test_basic6(int in, int ) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++, out++;
+  }
+}
+
+void test_basic7(int in, int ) {
+  if (in > 77) {
+out++;
+out++;
+  }
+  else
+out++;
+
+  out++;
+}
+
+void test_basic8(int in, int ) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+out++;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+out++;
+out++;
+  }
+
+  if (in % 2)
+out++;
+}
+
+void test_basic9(int in, int ) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+  }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int ) {
+  if (in > 77)
+if (in % 2)
+  out++;
+else
+  out--;
+  else
+if (in % 2)
+  out++;
+else
+  out--;
+
+}
+
+void test_basic11(int in, int ) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  }
+}
+
+void test_basic12(int in, int ) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+  }
+}
+
+void test_basic13(int in, int ) {
+  if (in > 77) {
+// Empty compound statement is not identical to null statement.
+  } else;
+}
+
+// We use a comparison that ignores redundant parentheses:
+void test_basic14(int in, int ) {
+  if (in > 77)
+out += 2;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+(out) += (2);
+}
+
+void test_basic15(int in, int ) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+((out += 2));
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out += 2;
+}
+
+// ..but does not apply additional simplifications: 
+void test_basic16(int in, int ) {
+  if (in > 77)
+out += 2;
+  else
+out += 1 + 1;
+}
+
+// ..and does not forget important parentheses:
+int test_basic17(int a, int b, int c, int mode) {
+  if (mode>8)
+return (a + b) * c;
+  else
+return a + b * c;
+}
+
+//===//
+
+#define PASTE_CODE(x) x
+
+void test_macro1(int in, int ) {
+  PASTE_CODE(

[PATCH] D55222: [AST] Assert that no statement/expression class is polymorphic

[Bruno Ricci via Phabricator via cfe-commits]

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

Add a `static_assert` checking that no statement/expression class is 
polymorphic.
People should use LLVM style RTTI instead.


Repository:
  rC Clang

https://reviews.llvm.org/D55222

Files:
  lib/AST/Stmt.cpp


Index: lib/AST/Stmt.cpp
===
--- lib/AST/Stmt.cpp
+++ lib/AST/Stmt.cpp
@@ -76,6 +76,14 @@
   return getStmtInfoTableEntry((StmtClass) StmtBits.sClass).Name;
 }
 
+// Check that no statement / expression class is polymorphic. LLVM style RTTI
+// should be used instead. If absolutely needed an exception can still be added
+// here by defining the appropriate macro (but please don't do this).
+#define STMT(CLASS, PARENT) \
+  static_assert(!std::is_polymorphic::value, \
+#CLASS "should not be polymorphic!");
+#include "clang/AST/StmtNodes.inc"
+
 void Stmt::PrintStats() {
   // Ensure the table is primed.
   getStmtInfoTableEntry(Stmt::NullStmtClass);


Index: lib/AST/Stmt.cpp
===
--- lib/AST/Stmt.cpp
+++ lib/AST/Stmt.cpp
@@ -76,6 +76,14 @@
   return getStmtInfoTableEntry((StmtClass) StmtBits.sClass).Name;
 }
 
+// Check that no statement / expression class is polymorphic. LLVM style RTTI
+// should be used instead. If absolutely needed an exception can still be added
+// here by defining the appropriate macro (but please don't do this).
+#define STMT(CLASS, PARENT) \
+  static_assert(!std::is_polymorphic::value, \
+#CLASS "should not be polymorphic!");
+#include "clang/AST/StmtNodes.inc"
+
 void Stmt::PrintStats() {
   // Ensure the table is primed.
   getStmtInfoTableEntry(Stmt::NullStmtClass);
___
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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:605
+  return strncmp(SM.getCharacterData(T1.getLocation()),
+ SM.getCharacterData(T2.getLocation()), T1.getLength()) == 0;
+}

This operation could overflow if `len(T1) > len(T2)` and `T2` is the last token 
of the file. I think `std::min(T1.getLength(), T2.getLength())` would be better.



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:608
+
+//Returns true if both LhsEpxr and RhsExpr are
+//macro expressions and they are expanded

Nit: Please add whitespace after // and use /// for the function documentation.



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:629
+
+  const char *LTokenPos = MB->getBufferStart() + LsrLocInfo.second;
+  const char *RTokenPos = MB->getBufferStart() + RsrLocInfo.second;

Please format with clang-format, some places seem to be a little off 
whitespace-wise.



Comment at: test/clang-tidy/misc-redundant-expression.cpp:109
 #define COND_OP_OTHER_MACRO 9
+#define COND_OP_THIRD_MACRO COND_OP_MACRO
 int TestConditional(int x, int y) {

dkrupp wrote:
> JonasToth wrote:
> > dkrupp wrote:
> > > JonasToth wrote:
> > > > Could you please add a test where the macro is `undef`ed and redefined 
> > > > to something else?
> > > I am not sure what you exactly suggest here. It should not matter what 
> > > COND_OP_THIRD_MACRO is defined to as we lexically compare tokens as they 
> > > are written in the original source code.
> > > 
> > > Could you be a bit more specific what test case you would like to add? 
> > *should* not matter is my point, please show that :)
> undef/define testcase added and passes. @JonasToth is this what you thought 
> of?
yup. thank you


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

[PATCH] D55221: [AST] Make ArrayTypeTraitExpr non-polymorphic.

[Bruno Ricci via Phabricator via cfe-commits]

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

`ArrayTypeTraitExpr` is the only expression class which is polymorphic.
As far as I can tell this is completely pointless.


Repository:
  rC Clang

https://reviews.llvm.org/D55221

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


Index: lib/AST/ExprCXX.cpp
===
--- lib/AST/ExprCXX.cpp
+++ lib/AST/ExprCXX.cpp
@@ -1443,5 +1443,3 @@
   void *Mem = C.Allocate(totalSizeToAlloc(NumArgs));
   return new (Mem) TypeTraitExpr(EmptyShell());
 }
-
-void ArrayTypeTraitExpr::anchor() {}
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -2455,8 +2455,6 @@
   /// The type being queried.
   TypeSourceInfo *QueriedType = nullptr;
 
-  virtual void anchor();
-
 public:
   friend class ASTStmtReader;
 
@@ -2474,8 +2472,6 @@
   explicit ArrayTypeTraitExpr(EmptyShell Empty)
   : Expr(ArrayTypeTraitExprClass, Empty), ATT(0) {}
 
-  virtual ~ArrayTypeTraitExpr() = default;
-
   SourceLocation getBeginLoc() const LLVM_READONLY { return Loc; }
   SourceLocation getEndLoc() const LLVM_READONLY { return RParen; }
 


Index: lib/AST/ExprCXX.cpp
===
--- lib/AST/ExprCXX.cpp
+++ lib/AST/ExprCXX.cpp
@@ -1443,5 +1443,3 @@
   void *Mem = C.Allocate(totalSizeToAlloc(NumArgs));
   return new (Mem) TypeTraitExpr(EmptyShell());
 }
-
-void ArrayTypeTraitExpr::anchor() {}
Index: include/clang/AST/ExprCXX.h
===
--- include/clang/AST/ExprCXX.h
+++ include/clang/AST/ExprCXX.h
@@ -2455,8 +2455,6 @@
   /// The type being queried.
   TypeSourceInfo *QueriedType = nullptr;
 
-  virtual void anchor();
-
 public:
   friend class ASTStmtReader;
 
@@ -2474,8 +2472,6 @@
   explicit ArrayTypeTraitExpr(EmptyShell Empty)
   : Expr(ArrayTypeTraitExprClass, Empty), ATT(0) {}
 
-  virtual ~ArrayTypeTraitExpr() = default;
-
   SourceLocation getBeginLoc() const LLVM_READONLY { return Loc; }
   SourceLocation getEndLoc() const LLVM_READONLY { return RParen; }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54903: [Sema] Improve static_assert diagnostics.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

Thank you both for the review !


Repository:
  rC Clang

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

https://reviews.llvm.org/D54903



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

Re: [PATCH] D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap

[Zachary Turner via cfe-commits]

I don’t think we really need this. isn’t Ilya’s solution in the other patch
already sufficient?
On Mon, Dec 3, 2018 at 7:34 AM Ivan Donchevskii via Phabricator <
revi...@reviews.llvm.org> wrote:

> yvvan added a comment.
>
> @ilya-biryukov
>
> Hm. What about another way around? - We have user include paths (-I) and
> report them to the filesystem. This means that we have specific paths under
> which nothing can be mmaped and everything else can be. In particular cases
> we can also report -isystem there. This is quite the same logic as current
> isVolatile parameter but is set only once for each path.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D54995/new/
>
> https://reviews.llvm.org/D54995
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+

ebevhan wrote:
> ebevhan wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > Running your test through GCC looks like the behavior matches here for 
> > > > C; can you also add a C++ test that demonstrates the behavior does not 
> > > > change?
> > > > 
> > > > https://godbolt.org/z/zRYDMG
> > > Strangely, the above godbolt link dropped the output windows, here's a 
> > > different link that shows the behavioral differences between C and C++ 
> > > mode in GCC: https://godbolt.org/z/R3zRHe
> > Hmm, I'll have a look at this.
> That gcc godbolt is a bit odd. The type of the bitfield expression in the C++ 
> example is `long` and not `int`, but in Clang, it's clearly being converted. 
> If I change the example a bit, we get this warning:
> ```
> :11:12: warning: format '%d' expects argument of type 'int', but 
> argument 2 has type 'long int' [-Wformat=]
>11 |   printf("%d", bf.a); // expected-warning {{format specifies type 
> 'long' but the argument has type 'int'}}
>   |   ~^   
>   ||  |
>   |intlong int
> ```
> But in Clang, we get a cast to `int`:
> ```
> | `-ImplicitCastExpr 0xd190748  'int' 
> |   `-ImplicitCastExpr 0xd190730  'long' 
> | `-MemberExpr 0xd190618  'long' lvalue bitfield .a 
> 0xd18f790
> |   `-DeclRefExpr 0xd1905f8  'struct bitfields':'bitfields' 
> lvalue Var 0xd18fa18 'bf' 'struct bitfields':'bitfields'
> ```
> 
> So gcc and Clang are doing things differently here.
> 
> The code in `isPromotableBitField` says:
> ```
>   // FIXME: C does not permit promotion of a 'long : 3' bitfield to int.
>   //We perform that promotion here to match GCC and C++.
> ```
> but clearly gcc isn't doing this in the C++ case. The comments also mention 
> some things about gcc bugs that Clang does not follow, but that's in 
> reference to a C DR.
C++ disallows the rank conversion from int to long as well. [conv.prom]p1 does 
not apply because `long int` has a higher rank than `int`, but [conv.prom]p5 
allows the promotion if the range of values is identical between the two types.

C makes this UB in several ways -- you can't have a bit-field whose type is 
something other than int, unsigned int, or _Bool (6.7.2.1p5) or promoting from 
types other than those (6.3.1.1p2), but otherwise matches the C++ behavior in 
terms of promotion (including the rank conversion).

You may have to dig further into what Clang is doing, but I would guess that 
the diagnostics should be triggered in both C and C++ similarly.

Ultimately, I'd like to see tests for cases where `sizeof(int) == 
sizeof(long)`, `sizeof(int) != sizeof(long)`, and variants for C and C++ of 
each.


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

https://reviews.llvm.org/D51211



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

[PATCH] D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

@ilya-biryukov

Hm. What about another way around? - We have user include paths (-I) and report 
them to the filesystem. This means that we have specific paths under which 
nothing can be mmaped and everything else can be. In particular cases we can 
also report -isystem there. This is quite the same logic as current isVolatile 
parameter but is set only once for each path.


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

https://reviews.llvm.org/D54995



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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: include/clang/Frontend/ASTUnit.h:581
 
+  unsigned getPreambleCounter() const { return PreambleCounter; }
+

NIT: `getPreambleCounterForTests()`? This is clearly an internal detail, would 
try giving it a name that discourages using it outside the testing code.



Comment at: lib/Frontend/ASTUnit.cpp:1369
 // Try again next time.
-PreambleRebuildCounter = 1;
+++PreambleCounter;
+PreambleRebuildCountdown = 1;

Why not increase this counter unconditionally for **every** error? (And a 
non-error case too, I guess)



Comment at: lib/Frontend/PrecompiledPreamble.cpp:471
+} else {
+  auto FilePath = PathNormalized(RB.first.begin(), RB.first.end());
+  OverridenFileBuffers[FilePath] = PreambleHash;

Could we avoid adding path normalization in this patch? Would it break anything?
This is clearly an important detail that preamble **might** get wrong, but I'd 
rather add it in a separate patch to avoid putting multiple things in the same 
patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D41005



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

Re: r348123 - [clang] Do not read from 'test/SemaCXX/Inputs' inside 'test/AST'

[Ilya Biryukov via cfe-commits]

No worries, this was easy to fix.
It's impossible to catch those in advance, we miss things like that all the
time.

On Mon, Dec 3, 2018 at 1:19 PM Aaron Ballman  wrote:

> On Mon, Dec 3, 2018 at 6:29 AM Ilya Biryukov via cfe-commits
>  wrote:
> >
> > Author: ibiryukov
> > Date: Mon Dec  3 03:26:35 2018
> > New Revision: 348123
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=348123=rev
> > Log:
> > [clang] Do not read from 'test/SemaCXX/Inputs' inside 'test/AST'
> >
> > Our integrate relies on test inputs being taken from the same diretory
> as the
> > test itself.
>
> Sorry about that! I was trying to avoid duplicating the same header
> file in two different places and didn't realize this would cause an
> issue.
>
> ~Aaron
>
> >
> > Added:
> > cfe/trunk/test/AST/Inputs/std-coroutine.h
> > Modified:
> > cfe/trunk/test/AST/coroutine-source-location-crash.cpp
> >
> > Added: cfe/trunk/test/AST/Inputs/std-coroutine.h
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/Inputs/std-coroutine.h?rev=348123=auto
> >
> ==
> > --- cfe/trunk/test/AST/Inputs/std-coroutine.h (added)
> > +++ cfe/trunk/test/AST/Inputs/std-coroutine.h Mon Dec  3 03:26:35 2018
> > @@ -0,0 +1,37 @@
> > +// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++14
> -fcoroutines-ts -fsyntax-only -Wignored-qualifiers -Wno-error=return-type
> -verify -fblocks -Wno-unreachable-code -Wno-unused-value
> > +#ifndef STD_COROUTINE_H
> > +#define STD_COROUTINE_H
> > +
> > +namespace std {
> > +namespace experimental {
> > +
> > +template 
> > +struct coroutine_traits { using promise_type = typename
> Ret::promise_type; };
> > +
> > +template 
> > +struct coroutine_handle {
> > +  static coroutine_handle from_address(void *);
> > +};
> > +template <>
> > +struct coroutine_handle {
> > +  template 
> > +  coroutine_handle(coroutine_handle);
> > +  static coroutine_handle from_address(void *);
> > +};
> > +
> > +struct suspend_always {
> > +  bool await_ready() { return false; }
> > +  void await_suspend(coroutine_handle<>) {}
> > +  void await_resume() {}
> > +};
> > +
> > +struct suspend_never {
> > +  bool await_ready() { return true; }
> > +  void await_suspend(coroutine_handle<>) {}
> > +  void await_resume() {}
> > +};
> > +
> > +} // namespace experimental
> > +} // namespace std
> > +
> > +#endif // STD_COROUTINE_H
> >
> > Modified: cfe/trunk/test/AST/coroutine-source-location-crash.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/coroutine-source-location-crash.cpp?rev=348123=348122=348123=diff
> >
> ==
> > --- cfe/trunk/test/AST/coroutine-source-location-crash.cpp (original)
> > +++ cfe/trunk/test/AST/coroutine-source-location-crash.cpp Mon Dec  3
> 03:26:35 2018
> > @@ -1,6 +1,6 @@
> >  // RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++14
> -fcoroutines-ts \
> >  // RUN:-fsyntax-only -ast-dump | FileCheck %s
> > -#include "../SemaCXX/Inputs/std-coroutine.h"
> > +#include "Inputs/std-coroutine.h"
> >
> >  using namespace std::experimental;
> >
> >
> >
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>


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

[PATCH] D54881: Prevent Clang-Format from editing leading whitespace on lines outside of the format range

[Russell McClellan via Phabricator via cfe-commits]

russellmcc added a comment.

Bump!  Thanks for your consideration.


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

https://reviews.llvm.org/D54881



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

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[Russell McClellan via Phabricator via cfe-commits]

russellmcc added a comment.

Bump!  Thanks again for your time.  As far as I can tell, it's ready for 
another round of review!


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

https://reviews.llvm.org/D40988



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

[PATCH] D55066: [ASan] Minor documentation fix: remove static linking limitation.

[Max Moroz via Phabricator via cfe-commits]

Dor1s added a comment.

In D55066#1315365 , @eugenis wrote:

> Sorry for the delay.
>  This is wrong, static linking is NOT supported.
>  You could be confusing it with static linking of asan runtime library to an 
> executable - that is and has always been the default.


Thanks, @eugenis! Should I change the message to something like "ASan runtime 
cannot be built with static linking."? Does it sound correct and perhaps less 
confusing?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55066



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

[PATCH] D52984: [analyzer] Checker reviewer's checklist

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 176403.
xazax.hun marked 4 inline comments as done.
xazax.hun added a comment.

- Addressed further comments.


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

https://reviews.llvm.org/D52984

Files:
  www/analyzer/checker_dev_manual.html

Index: www/analyzer/checker_dev_manual.html
===
--- www/analyzer/checker_dev_manual.html
+++ www/analyzer/checker_dev_manual.html
@@ -675,6 +675,111 @@
 (gdb) p C.getPredecessor()->getCodeDecl().getBody()->dump()
 
 
+Making Your Checker Better
+
+User facing documentation is important for adoption! Make sure the checker list is updated
+at the homepage of the analyzer. Also ensure the description is clear to
+non-analyzer-developers in Checkers.td.
+Warning and note messages should be clear and easy to understand, even if a bit long.
+
+  Messages should start with a capital letter (unlike Clang warnings!) and should not
+  end with ..
+  Articles are usually omitted, eg. Dereference of a null pointer ->
+  Dereference of null pointer.
+  Introduce BugReporterVisitors to emit additional notes that explain the warning
+  to the user better. There are some existing visitors that might be useful for your check,
+  e.g. trackNullOrUndefValue. For example, SimpleStreamChecker should highlight
+  the event of opening the file when reporting a file descriptor leak.
+
+If the check tracks anything in the program state, it needs to implement the
+checkDeadSymbolscallback to clean the state up.
+The check should conservatively assume that the program is correct when a tracked symbol
+is passed to a function that is unknown to the analyzer.
+checkPointerEscape callback could help you handle that case.
+Use safe and convenient APIs!
+
+  Always use CheckerContext::generateErrorNode and
+CheckerContext::generateNonFatalErrorNode for emitting bug reports.
+Most importantly, never emit report against CheckerContext::getPredecessor.
+  Prefer checkPreCall and checkPostCall to
+checkPreStmtCallExpr and checkPostStmtCallExpr.
+  Use CallDescription to detect hardcoded API calls in the program.
+  Simplify C.getState()->getSVal(E, C.getLocationContext()) to C.getSVal(E).
+
+Common sources of crashes:
+
+  CallEvent::getOriginExpr is nullable - for example, it returns null for an
+automatic destructor of a variable. The same applies to some values generated while the
+call was modeled, eg. SymbolConjured::getStmt is nullable.
+  CallEvent::getDecl is nullable - for example, it returns null for a
+  call of symbolic function pointer.
+  addTransition, generateSink, generateNonFatalErrorNode,
+generateErrorNode are nullable because you can transition to a node that you have already visited.
+  Methods of CallExpr/FunctionDecl/CallEvent that
+return arguments crash when the argument is out-of-bounds. If you checked the function name,
+it doesn't mean that the function has the expected number of arguments!
+Which is why you should use CallDescription.
+  Nullability of different entities within different kinds of symbols and regions is usually
+  documented via assertions in their constructors.
+  NamedDecl::getName will fail if the name of the declaration is not a single token,
+e.g. for destructors. You could use NamedDecl::getNameAsString for those cases.
+Note that this method is much slower and should be used sparringly, e.g. only when generating reports
+but not during analysis.
+  Is -analyzer-checker=core included in all test RUN: lines? It was never supported
+to run the analyzer with the core checks disabled. It might cause unexpected behavior and
+crashes. You should do all your testing with the core checks enabled.
+
+
+Patterns that you should most likely avoid even if they're not technically wrong:
+
+  BugReporterVisitor should most likely not match the AST of the current program point
+  to decide when to emit a note. It is much easier to determine that by observing changes in
+  the program state.
+  In State->getSVal(Region), if Region is not known to be a TypedValueRegion
+  and the optional type argument is not specified, the checker may accidentally try to dereference a
+  void pointer.
+  Checker logic should not depend on whether a certain value is a Loc or NonLoc.
+It should be immediately obvious whether the SVal is a Loc or a
+NonLoc depending on the AST that is being checked. Checking whether a value
+is Loc or Unknown/Undefined or whether the value is
+NonLoc or Unknown/Undefined is totally fine.
+  New symbols should not be constructed in the checker via direct calls to SymbolManager,
+unless they are of SymbolMetadata class tagged by the checker,
+or they represent newly created values such as the return value in evalCall.
+For modeling arithmetic/bitwise/comparison operations, SValBuilder 

[PATCH] D55139: [clangd] Avoid memory-mapping files on Windows

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE348147: [clangd] Avoid memory-mapping files on Windows 
(authored by ibiryukov, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55139?vs=176382=176401#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55139

Files:
  clangd/CMakeLists.txt
  clangd/FSProvider.cpp
  clangd/FSProvider.h

Index: clangd/FSProvider.cpp
===
--- clangd/FSProvider.cpp
+++ clangd/FSProvider.cpp
@@ -0,0 +1,85 @@
+//===--- FSProvider.cpp - VFS provider for ClangdServer ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FSProvider.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include 
+
+using namespace llvm;
+
+namespace clang {
+namespace clangd {
+
+namespace {
+/// Always opens files in the underlying filesystem as "volatile", meaning they
+/// won't be memory-mapped. This avoid locking the files on Windows.
+class VolatileFileSystem : public llvm::vfs::ProxyFileSystem {
+public:
+  explicit VolatileFileSystem(llvm::IntrusiveRefCntPtr FS)
+  : ProxyFileSystem(std::move(FS)) {}
+
+  llvm::ErrorOr>
+  openFileForRead(const Twine ) override {
+SmallString<128> Path;
+InPath.toVector(Path);
+
+auto File = getUnderlyingFS().openFileForRead(Path);
+if (!File)
+  return File;
+// Try to guess preamble files, they can be memory-mapped even on Windows as
+// clangd has exclusive access to those.
+StringRef FileName = llvm::sys::path::filename(Path);
+if (FileName.startswith("preamble-") && FileName.endswith(".pch"))
+  return File;
+return std::unique_ptr(
+new VolatileFile(std::move(*File)));
+  }
+
+private:
+  class VolatileFile : public vfs::File {
+  public:
+VolatileFile(std::unique_ptr Wrapped)
+: Wrapped(std::move(Wrapped)) {
+  assert(this->Wrapped);
+}
+
+virtual llvm::ErrorOr>
+getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
+  bool /*IsVolatile*/) override {
+  return Wrapped->getBuffer(Name, FileSize, RequiresNullTerminator,
+/*IsVolatile=*/true);
+}
+
+llvm::ErrorOr status() override { return Wrapped->status(); }
+llvm::ErrorOr getName() override { return Wrapped->getName(); }
+std::error_code close() override { return Wrapped->close(); }
+
+  private:
+std::unique_ptr Wrapped;
+  };
+};
+} // namespace
+
+llvm::IntrusiveRefCntPtr
+clang::clangd::RealFileSystemProvider::getFileSystem() const {
+// Avoid using memory-mapped files on Windows, they cause file locking issues.
+// FIXME: Try to use a similar approach in Sema instead of relying on
+//propagation of the 'isVolatile' flag through all layers.
+#ifdef _WIN32
+  return new VolatileFSProvider(vfs::getRealFileSystem());
+#else
+  return vfs::getRealFileSystem();
+#endif
+}
+} // namespace clangd
+} // namespace clang
Index: clangd/CMakeLists.txt
===
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -23,6 +23,7 @@
   FindSymbols.cpp
   FileDistance.cpp
   FS.cpp
+  FSProvider.cpp
   FuzzyMatch.cpp
   GlobalCompilationDatabase.cpp
   Headers.cpp
Index: clangd/FSProvider.h
===
--- clangd/FSProvider.h
+++ clangd/FSProvider.h
@@ -33,9 +33,7 @@
 public:
   // FIXME: returns the single real FS instance, which is not threadsafe.
   llvm::IntrusiveRefCntPtr
-  getFileSystem() const override {
-return llvm::vfs::getRealFileSystem();
-  }
+  getFileSystem() const override;
 };
 
 } // namespace clangd
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D41005



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

[clang-tools-extra] r348147 - [clangd] Avoid memory-mapping files on Windows

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Mon Dec  3 07:21:49 2018
New Revision: 348147

URL: http://llvm.org/viewvc/llvm-project?rev=348147=rev
Log:
[clangd] Avoid memory-mapping files on Windows

Summary:
Memory-mapping files on Windows leads to them being locked and prevents
editors from saving changes to those files on disk. This is fine for the
compiler, but not acceptable for an interactive tool like clangd.
Therefore, we choose to avoid using memory-mapped files on Windows.

Reviewers: hokein, kadircet

Reviewed By: kadircet

Subscribers: yvvan, zturner, nik, malaperle, mgorny, ioeric, MaskRay, jkorous, 
arphaman, cfe-commits

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

Added:
clang-tools-extra/trunk/clangd/FSProvider.cpp
Modified:
clang-tools-extra/trunk/clangd/CMakeLists.txt
clang-tools-extra/trunk/clangd/FSProvider.h

Modified: clang-tools-extra/trunk/clangd/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CMakeLists.txt?rev=348147=348146=348147=diff
==
--- clang-tools-extra/trunk/clangd/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clangd/CMakeLists.txt Mon Dec  3 07:21:49 2018
@@ -23,6 +23,7 @@ add_clang_library(clangDaemon
   FindSymbols.cpp
   FileDistance.cpp
   FS.cpp
+  FSProvider.cpp
   FuzzyMatch.cpp
   GlobalCompilationDatabase.cpp
   Headers.cpp

Added: clang-tools-extra/trunk/clangd/FSProvider.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FSProvider.cpp?rev=348147=auto
==
--- clang-tools-extra/trunk/clangd/FSProvider.cpp (added)
+++ clang-tools-extra/trunk/clangd/FSProvider.cpp Mon Dec  3 07:21:49 2018
@@ -0,0 +1,85 @@
+//===--- FSProvider.cpp - VFS provider for ClangdServer 
---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FSProvider.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include 
+
+using namespace llvm;
+
+namespace clang {
+namespace clangd {
+
+namespace {
+/// Always opens files in the underlying filesystem as "volatile", meaning they
+/// won't be memory-mapped. This avoid locking the files on Windows.
+class VolatileFileSystem : public llvm::vfs::ProxyFileSystem {
+public:
+  explicit VolatileFileSystem(llvm::IntrusiveRefCntPtr FS)
+  : ProxyFileSystem(std::move(FS)) {}
+
+  llvm::ErrorOr>
+  openFileForRead(const Twine ) override {
+SmallString<128> Path;
+InPath.toVector(Path);
+
+auto File = getUnderlyingFS().openFileForRead(Path);
+if (!File)
+  return File;
+// Try to guess preamble files, they can be memory-mapped even on Windows 
as
+// clangd has exclusive access to those.
+StringRef FileName = llvm::sys::path::filename(Path);
+if (FileName.startswith("preamble-") && FileName.endswith(".pch"))
+  return File;
+return std::unique_ptr(
+new VolatileFile(std::move(*File)));
+  }
+
+private:
+  class VolatileFile : public vfs::File {
+  public:
+VolatileFile(std::unique_ptr Wrapped)
+: Wrapped(std::move(Wrapped)) {
+  assert(this->Wrapped);
+}
+
+virtual llvm::ErrorOr>
+getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
+  bool /*IsVolatile*/) override {
+  return Wrapped->getBuffer(Name, FileSize, RequiresNullTerminator,
+/*IsVolatile=*/true);
+}
+
+llvm::ErrorOr status() override { return Wrapped->status(); }
+llvm::ErrorOr getName() override { return Wrapped->getName(); 
}
+std::error_code close() override { return Wrapped->close(); }
+
+  private:
+std::unique_ptr Wrapped;
+  };
+};
+} // namespace
+
+llvm::IntrusiveRefCntPtr
+clang::clangd::RealFileSystemProvider::getFileSystem() const {
+// Avoid using memory-mapped files on Windows, they cause file locking issues.
+// FIXME: Try to use a similar approach in Sema instead of relying on
+//propagation of the 'isVolatile' flag through all layers.
+#ifdef _WIN32
+  return new VolatileFSProvider(vfs::getRealFileSystem());
+#else
+  return vfs::getRealFileSystem();
+#endif
+}
+} // namespace clangd
+} // namespace clang

Modified: clang-tools-extra/trunk/clangd/FSProvider.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FSProvider.h?rev=348147=348146=348147=diff
==
--- clang-tools-extra/trunk/clangd/FSProvider.h (original)
+++ clang-tools-extra/trunk/clangd/FSProvider.h Mon Dec  3 07:21:49 2018
@@ -33,9 +33,7 @@ class 

[PATCH] D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D54995#1316476 , @yvvan wrote:

> I don't think removing the flag is a good idea since I can easily assume 
> cases when user wants mmap and is ready to encounter locks. In our case it 
> can be an IDE option which behavior to choose.


Would that option be useful if changing it forces the files user is editing in 
the IDE to be indefinitely locked and stops them from saving the files?
Anyway, even if you feel this behavior should be configurable, that's totally 
fine - I was referring to removing the flag from the vfs::FileSystem 
**interface**, possibly replacing it with a filesystem implementation that 
would handle the same thing at a lower level.
This flag is not useful to any of the filesystem implementation, except 
RealFileSystem, which actually has to deal with opening OS files.

> The ideal solution in my opinion would be to have isSystem(filename) in 
> FileSystem class. And is based on system directories with which ASTUnit feeds 
> the FileSystem for example.

I would disagree that `isSystem()` is a property of the filesystem. The 
filesystem abstraction is responsible for providing access to the files and 
their contents, why should classification of the files be filesystem-dependent? 
It's the wrong layer for the `isSystem()` check.
E.g. imagine implementing a filesystem that gets files from the network. The 
concerns filesystem is responsible for clearly include doing network IO. But 
why should the system/user classification of the files be different for it? 
What policy decisions does it have to make there? Clearly some other should do 
this classification and it's clearly independent of the filesystem being used.
OTOH, passing whether we want to open the files as memory-mapped does not sound 
completely crazy. But it would still be nice to avoid having it in the 
interface, since we **only** need it to workaround limitations specific to 
Windows + interactive editors, which is only a small subset of LLVM-based tools 
uses.


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

https://reviews.llvm.org/D54995



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

[PATCH] D53866: [Preamble] Fix preamble for circular #includes

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53866



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

[PATCH] D54903: [Sema] Improve static_assert diagnostics.

[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!




Comment at: lib/Sema/SemaTemplate.cpp:3055
 
+// Print a diagnostic for the failing static_assert expression. Defaults to
+// pretty-printing the expression.

courbet wrote:
> aaron.ballman wrote:
> > courbet wrote:
> > > aaron.ballman wrote:
> > > > Comment is a bit out of date as this is no longer specific to 
> > > > `static_assert`.
> > > > 
> > > > It looks like this may also change the behavior of enable_if diagnostic 
> > > > reporting. Do you need to update any of those tests from this change? 
> > > > If not, can you devise some tests for that case as well to show that 
> > > > this isn't just a static_assert behavior? (Do a search for 
> > > > `findFailedBooleanCondition` to find where the behavior has changed.)
> > > Nope, unfortunately the code that calls this is actually untested...
> > > I could not find a test that actually enters the:
> > > ```
> > > if (TypeAliasTemplateDecl *AliasTemplate =
> > >   dyn_cast(Template))
> > > ```
> > > in `Sema::CheckTemplateIdType`.
> > > 
> > > I stopped at the following insanity:
> > > 
> > > ```
> > > // RUN: %clang_cc1 -std=c++11 -verify %s
> > > 
> > > template 
> > > struct is_same {
> > >   enum { value = 0 };
> > > };
> > > 
> > > template  struct is_same {
> > >   enum { value = 1 };
> > > };
> > > 
> > > struct Dispatch {
> > >   template  using SameAs = is_same;
> > > };
> > > 
> > > template 
> > > struct S {
> > >   template  using same = typename DispatchT::template 
> > > SameAs;
> > > 
> > >   template 
> > >   static void foo() __attribute__((enable_if(same::value, "")));
> > > 
> > > };
> > > 
> > > void runFoo() {
> > >   // S::foo();
> > >   S::foo();
> > > 
> > > }
> > > ```
> > > 
> > > This one exercises the code up to `if (CanonType.isNull()) {`, but I'm 
> > > not sure how to get a null type here.
> > > 
> > How about this:
> > ```
> > namespace boost {
> >   template struct enable_if {};
> >   template struct enable_if { typedef T type; };
> > }
> > 
> > template struct NonTemplateFunction {
> >   typename boost::enable_if::type f();
> > };
> > 
> > template 
> > struct Bobble {
> >   using type = T;
> > };
> > 
> > struct Frobble {
> >   using type = char;
> > };
> > NonTemplateFunction::type> NTFC;
> > ```
> > That should trigger the path in `Sema::CheckTypenameType()`, I believe.
> This triggers:
> ```
> else if (ClassTemplateDecl *ClassTemplate
>= dyn_cast(Template))
> ```
> 
> Modifying it to:
> ```
> namespace boost {
>   template struct enable_if {};
>   template struct enable_if { typedef T type; };
> }
> 
> template struct NonTemplateFunction {
>   template  using toto = typename boost::enable_if 4, int>::type;
>   toto f();
> };
> 
> template 
> struct Bobble {
>   using type = T;
> };
> 
> struct Frobble {
>   using type = char;
> };
> NonTemplateFunction::type> NTFC;
> ```
> 
> triggers the same path as my previous example, but not the `IsNull()` test.
Aw crud, well, we tried.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54903



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

[PATCH] D55062: [clangd] Partition include graph on auto-index.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Everything looks good, just a single important request about testing the 
included files do not have any edges in the resulting graph




Comment at: clangd/index/Background.cpp:79
+
+  // Since the strings in direct includes are references to the keys of the
+  // fullgraph, we need to create entries in our new sub-graph for those

NIT: the comment could probably be simplified to something like `URIs inside 
nodes must point into the keys of the same IncludeGraph`



Comment at: clangd/index/Background.cpp:386
 
-  log("Indexed {0} ({1} symbols, {2} refs)", Inputs.CompileCommand.Filename,
-  Symbols.size(), Refs.numRefs());
-  SPAN_ATTACH(Tracer, "symbols", int(Symbols.size()));
-  SPAN_ATTACH(Tracer, "refs", int(Refs.numRefs()));
-  IndexFileIn Index;
-  Index.Symbols = std::move(Symbols);
-  Index.Refs = std::move(Refs);
+  log("Indexed {0} ({1} symbols, {2} refs, {3} sources)",
+  Inputs.CompileCommand.Filename, Index.Symbols->size(),

"sources" might be a bit confusing. I'd probably use "files" instead, but that 
doesn't look ideal too.



Comment at: unittests/clangd/BackgroundIndexTests.cpp:197
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_TRUE(ShardSource->Sources);
+  EXPECT_EQ(ShardSource->Sources->size(), 2U); // A.cc, A.h

Maybe check the full shape of the graph? E.g. also add a check that `A.h` does 
not have any includes and its digest is populated.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55062



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

[PATCH] D54903: [Sema] Improve static_assert diagnostics.

[Clement Courbet via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaTemplate.cpp:3055
 
+// Print a diagnostic for the failing static_assert expression. Defaults to
+// pretty-printing the expression.

aaron.ballman wrote:
> courbet wrote:
> > aaron.ballman wrote:
> > > Comment is a bit out of date as this is no longer specific to 
> > > `static_assert`.
> > > 
> > > It looks like this may also change the behavior of enable_if diagnostic 
> > > reporting. Do you need to update any of those tests from this change? If 
> > > not, can you devise some tests for that case as well to show that this 
> > > isn't just a static_assert behavior? (Do a search for 
> > > `findFailedBooleanCondition` to find where the behavior has changed.)
> > Nope, unfortunately the code that calls this is actually untested...
> > I could not find a test that actually enters the:
> > ```
> > if (TypeAliasTemplateDecl *AliasTemplate =
> >   dyn_cast(Template))
> > ```
> > in `Sema::CheckTemplateIdType`.
> > 
> > I stopped at the following insanity:
> > 
> > ```
> > // RUN: %clang_cc1 -std=c++11 -verify %s
> > 
> > template 
> > struct is_same {
> >   enum { value = 0 };
> > };
> > 
> > template  struct is_same {
> >   enum { value = 1 };
> > };
> > 
> > struct Dispatch {
> >   template  using SameAs = is_same;
> > };
> > 
> > template 
> > struct S {
> >   template  using same = typename DispatchT::template SameAs;
> > 
> >   template 
> >   static void foo() __attribute__((enable_if(same::value, "")));
> > 
> > };
> > 
> > void runFoo() {
> >   // S::foo();
> >   S::foo();
> > 
> > }
> > ```
> > 
> > This one exercises the code up to `if (CanonType.isNull()) {`, but I'm not 
> > sure how to get a null type here.
> > 
> How about this:
> ```
> namespace boost {
>   template struct enable_if {};
>   template struct enable_if { typedef T type; };
> }
> 
> template struct NonTemplateFunction {
>   typename boost::enable_if::type f();
> };
> 
> template 
> struct Bobble {
>   using type = T;
> };
> 
> struct Frobble {
>   using type = char;
> };
> NonTemplateFunction::type> NTFC;
> ```
> That should trigger the path in `Sema::CheckTypenameType()`, I believe.
This triggers:
```
else if (ClassTemplateDecl *ClassTemplate
   = dyn_cast(Template))
```

Modifying it to:
```
namespace boost {
  template struct enable_if {};
  template struct enable_if { typedef T type; };
}

template struct NonTemplateFunction {
  template  using toto = typename boost::enable_if::type;
  toto f();
};

template 
struct Bobble {
  using type = T;
};

struct Frobble {
  using type = char;
};
NonTemplateFunction::type> NTFC;
```

triggers the same path as my previous example, but not the `IsNull()` test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54903



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

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348145: [AST][Sema] Remove CallExpr::setNumArgs (authored by 
brunoricci, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54902?vs=175434=176394#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54902

Files:
  cfe/trunk/include/clang/AST/Expr.h
  cfe/trunk/include/clang/AST/ExprCXX.h
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/lib/Sema/SemaOverload.cpp
  cfe/trunk/lib/Serialization/ASTReaderStmt.cpp

Index: cfe/trunk/include/clang/AST/Expr.h
===
--- cfe/trunk/include/clang/AST/Expr.h
+++ cfe/trunk/include/clang/AST/Expr.h
@@ -2416,11 +2416,12 @@
   // These versions of the constructor are for derived classes.
   CallExpr(const ASTContext , StmtClass SC, Expr *fn,
ArrayRef preargs, ArrayRef args, QualType t,
-   ExprValueKind VK, SourceLocation rparenloc);
+   ExprValueKind VK, SourceLocation rparenloc, unsigned MinNumArgs = 0);
   CallExpr(const ASTContext , StmtClass SC, Expr *fn, ArrayRef args,
-   QualType t, ExprValueKind VK, SourceLocation rparenloc);
+   QualType t, ExprValueKind VK, SourceLocation rparenloc,
+   unsigned MinNumArgs = 0);
   CallExpr(const ASTContext , StmtClass SC, unsigned NumPreArgs,
-   EmptyShell Empty);
+   unsigned NumArgs, EmptyShell Empty);
 
   Stmt *getPreArg(unsigned i) {
 assert(i < getNumPreArgs() && "Prearg access out of range!");
@@ -2438,11 +2439,14 @@
   unsigned getNumPreArgs() const { return CallExprBits.NumPreArgs; }
 
 public:
-  CallExpr(const ASTContext& C, Expr *fn, ArrayRef args, QualType t,
-   ExprValueKind VK, SourceLocation rparenloc);
+  /// Build a call expression. MinNumArgs specifies the minimum number of
+  /// arguments. The actual number of arguments will be the greater of
+  /// args.size() and MinNumArgs.
+  CallExpr(const ASTContext , Expr *fn, ArrayRef args, QualType t,
+   ExprValueKind VK, SourceLocation rparenloc, unsigned MinNumArgs = 0);
 
   /// Build an empty call expression.
-  CallExpr(const ASTContext , StmtClass SC, EmptyShell Empty);
+  CallExpr(const ASTContext , unsigned NumArgs, EmptyShell Empty);
 
   const Expr *getCallee() const { return cast(SubExprs[FN]); }
   Expr *getCallee() { return cast(SubExprs[FN]); }
@@ -2488,10 +2492,18 @@
 SubExprs[Arg+getNumPreArgs()+PREARGS_START] = ArgExpr;
   }
 
-  /// setNumArgs - This changes the number of arguments present in this call.
-  /// Any orphaned expressions are deleted by this, and any new operands are set
-  /// to null.
-  void setNumArgs(const ASTContext& C, unsigned NumArgs);
+  /// Reduce the number of arguments in this call expression. This is used for
+  /// example during error recovery to drop extra arguments. There is no way
+  /// to perform the opposite because: 1.) We don't track how much storage
+  /// we have for the argument array 2.) This would potentially require growing
+  /// the argument array, something we cannot support since the arguments will
+  /// be stored in a trailing array in the future.
+  /// (TODO: update this comment when this is done).
+  void shrinkNumArgs(unsigned NewNumArgs) {
+assert((NewNumArgs <= NumArgs) &&
+   "shrinkNumArgs cannot increase the number of arguments!");
+NumArgs = NewNumArgs;
+  }
 
   typedef ExprIterator arg_iterator;
   typedef ConstExprIterator const_arg_iterator;
Index: cfe/trunk/include/clang/AST/ExprCXX.h
===
--- cfe/trunk/include/clang/AST/ExprCXX.h
+++ cfe/trunk/include/clang/AST/ExprCXX.h
@@ -98,8 +98,10 @@
 Range = getSourceRangeImpl();
   }
 
-  explicit CXXOperatorCallExpr(ASTContext& C, EmptyShell Empty)
-  : CallExpr(C, CXXOperatorCallExprClass, Empty) {}
+  explicit CXXOperatorCallExpr(ASTContext , unsigned NumArgs,
+   EmptyShell Empty)
+  : CallExpr(C, CXXOperatorCallExprClass, /*NumPreArgs=*/0, NumArgs,
+ Empty) {}
 
   /// Returns the kind of overloaded operator that this
   /// expression refers to.
@@ -163,12 +165,13 @@
 /// the object argument).
 class CXXMemberCallExpr : public CallExpr {
 public:
-  CXXMemberCallExpr(ASTContext , Expr *fn, ArrayRef args,
-QualType t, ExprValueKind VK, SourceLocation RP)
-  : CallExpr(C, CXXMemberCallExprClass, fn, args, t, VK, RP) {}
+  CXXMemberCallExpr(ASTContext , Expr *fn, ArrayRef args, QualType t,
+ExprValueKind VK, SourceLocation RP,
+unsigned MinNumArgs = 0)
+  : CallExpr(C, CXXMemberCallExprClass, fn, args, t, VK, RP, MinNumArgs) {}
 
-  CXXMemberCallExpr(ASTContext , EmptyShell Empty)
-  : CallExpr(C, CXXMemberCallExprClass, Empty) {}
+  

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

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



Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+

ebevhan wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Running your test through GCC looks like the behavior matches here for C; 
> > > can you also add a C++ test that demonstrates the behavior does not 
> > > change?
> > > 
> > > https://godbolt.org/z/zRYDMG
> > Strangely, the above godbolt link dropped the output windows, here's a 
> > different link that shows the behavioral differences between C and C++ mode 
> > in GCC: https://godbolt.org/z/R3zRHe
> Hmm, I'll have a look at this.
That gcc godbolt is a bit odd. The type of the bitfield expression in the C++ 
example is `long` and not `int`, but in Clang, it's clearly being converted. If 
I change the example a bit, we get this warning:
```
:11:12: warning: format '%d' expects argument of type 'int', but 
argument 2 has type 'long int' [-Wformat=]
   11 |   printf("%d", bf.a); // expected-warning {{format specifies type 
'long' but the argument has type 'int'}}
  |   ~^   
  ||  |
  |intlong int
```
But in Clang, we get a cast to `int`:
```
| `-ImplicitCastExpr 0xd190748  'int' 
|   `-ImplicitCastExpr 0xd190730  'long' 
| `-MemberExpr 0xd190618  'long' lvalue bitfield .a 
0xd18f790
|   `-DeclRefExpr 0xd1905f8  'struct bitfields':'bitfields' 
lvalue Var 0xd18fa18 'bf' 'struct bitfields':'bitfields'
```

So gcc and Clang are doing things differently here.

The code in `isPromotableBitField` says:
```
  // FIXME: C does not permit promotion of a 'long : 3' bitfield to int.
  //We perform that promotion here to match GCC and C++.
```
but clearly gcc isn't doing this in the C++ case. The comments also mention 
some things about gcc bugs that Clang does not follow, but that's in reference 
to a C DR.


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

https://reviews.llvm.org/D51211



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

r348145 - [AST][Sema] Remove CallExpr::setNumArgs

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Mon Dec  3 06:54:03 2018
New Revision: 348145

URL: http://llvm.org/viewvc/llvm-project?rev=348145=rev
Log:
[AST][Sema] Remove CallExpr::setNumArgs

CallExpr::setNumArgs is the only thing that prevents storing the arguments
in a trailing array. There is only 3 places in Sema where setNumArgs is called.
D54900 dealt with one of them.

This patch remove the other two calls to setNumArgs in ConvertArgumentsForCall.
To do this we do the following changes:

1.) Replace the first call to setNumArgs by an assertion since we are moving the
responsability to allocate enough space for the arguments from
Sema::ConvertArgumentsForCall to its callers
(which are Sema::BuildCallToMemberFunction, and Sema::BuildResolvedCallExpr).

2.) Add a new member function CallExpr::shrinkNumArgs, which can only be used
to drop arguments and then replace the second call to setNumArgs by
shrinkNumArgs.

3.) Add a new defaulted parameter MinNumArgs to CallExpr and its derived
classes which specifies a minimum number of argument slots to allocate.
The actual number of arguments slots allocated will be
max(number of args, MinNumArgs) with the extra args nulled. Note that
after the creation of the call expression all of the arguments will be
non-null. It is just during the creation of the call expression that some of
the last arguments can be temporarily null, until filled by default arguments.

4.) Update Sema::BuildCallToMemberFunction by passing the number of parameters
in the function prototype to the constructor of CXXMemberCallExpr. Here the
change is pretty straightforward.

5.) Update Sema::BuildResolvedCallExpr. Here the change is more complicated
since the type-checking for the function type was done after the creation of
the call expression. We need to move this before the creation of the call
expression, and then pass the number of parameters in the function prototype
(if any) to the constructor of the call expression.

6.) Update the deserialization of CallExpr and its derived classes.

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

Reviewed By: aaron.ballman


Modified:
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/include/clang/AST/ExprCXX.h
cfe/trunk/lib/AST/Expr.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/lib/Sema/SemaOverload.cpp
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp

Modified: cfe/trunk/include/clang/AST/Expr.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=348145=348144=348145=diff
==
--- cfe/trunk/include/clang/AST/Expr.h (original)
+++ cfe/trunk/include/clang/AST/Expr.h Mon Dec  3 06:54:03 2018
@@ -2416,11 +2416,12 @@ protected:
   // These versions of the constructor are for derived classes.
   CallExpr(const ASTContext , StmtClass SC, Expr *fn,
ArrayRef preargs, ArrayRef args, QualType t,
-   ExprValueKind VK, SourceLocation rparenloc);
+   ExprValueKind VK, SourceLocation rparenloc, unsigned MinNumArgs = 
0);
   CallExpr(const ASTContext , StmtClass SC, Expr *fn, ArrayRef args,
-   QualType t, ExprValueKind VK, SourceLocation rparenloc);
+   QualType t, ExprValueKind VK, SourceLocation rparenloc,
+   unsigned MinNumArgs = 0);
   CallExpr(const ASTContext , StmtClass SC, unsigned NumPreArgs,
-   EmptyShell Empty);
+   unsigned NumArgs, EmptyShell Empty);
 
   Stmt *getPreArg(unsigned i) {
 assert(i < getNumPreArgs() && "Prearg access out of range!");
@@ -2438,11 +2439,14 @@ protected:
   unsigned getNumPreArgs() const { return CallExprBits.NumPreArgs; }
 
 public:
-  CallExpr(const ASTContext& C, Expr *fn, ArrayRef args, QualType t,
-   ExprValueKind VK, SourceLocation rparenloc);
+  /// Build a call expression. MinNumArgs specifies the minimum number of
+  /// arguments. The actual number of arguments will be the greater of
+  /// args.size() and MinNumArgs.
+  CallExpr(const ASTContext , Expr *fn, ArrayRef args, QualType t,
+   ExprValueKind VK, SourceLocation rparenloc, unsigned MinNumArgs = 
0);
 
   /// Build an empty call expression.
-  CallExpr(const ASTContext , StmtClass SC, EmptyShell Empty);
+  CallExpr(const ASTContext , unsigned NumArgs, EmptyShell Empty);
 
   const Expr *getCallee() const { return cast(SubExprs[FN]); }
   Expr *getCallee() { return cast(SubExprs[FN]); }
@@ -2488,10 +2492,18 @@ public:
 SubExprs[Arg+getNumPreArgs()+PREARGS_START] = ArgExpr;
   }
 
-  /// setNumArgs - This changes the number of arguments present in this call.
-  /// Any orphaned expressions are deleted by this, and any new operands are 
set
-  /// to null.
-  void setNumArgs(const ASTContext& C, unsigned NumArgs);
+  /// Reduce the number of arguments in this call expression. This is used for
+  /// example during error recovery to drop extra arguments. There is no way
+  /// to perform the opposite because: 1.) We don't track how much storage
+  /// we 

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D54630#1315168 , @arphaman wrote:

> So far everything looks fine, but I have to rerun a couple of things due to 
> infrastructural issues. I should have the final results next Monday.


Thanks for the update. Will be waiting for the final results.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54630



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

[PATCH] D53812: [Analyzer] Iterator Checker - Forbid decrements past the begin() and increments past the end() of containers

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 176389.
baloghadamsoftware added a comment.

One error message slightly updated.


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

https://reviews.llvm.org/D53812

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/iterator-range.cpp

Index: test/Analysis/iterator-range.cpp
===
--- test/Analysis/iterator-range.cpp
+++ test/Analysis/iterator-range.cpp
@@ -23,34 +23,13 @@
 
 void simple_bad_end(const std::vector ) {
   auto i = v.end();
-  *i; // expected-warning{{Iterator accessed outside of its range}}
-}
-
-void simple_good_begin(const std::vector ) {
-  auto i = v.begin();
-  if (i != v.begin()) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_good_begin_negated(const std::vector ) {
-  auto i = v.begin();
-  if (!(i == v.begin())) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_bad_begin(const std::vector ) {
-  auto i = v.begin();
-  *--i; // expected-warning{{Iterator accessed outside of its range}}
+  *i; // expected-warning{{Past-the-end iterator dereferenced}}
 }
 
 void copy(const std::vector ) {
   auto i1 = v.end();
   auto i2 = i1;
-  *i2; // expected-warning{{Iterator accessed outside of its range}}
+  *i2; // expected-warning{{Past-the-end iterator dereferenced}}
 }
 
 void decrease(const std::vector ) {
@@ -70,7 +49,7 @@
   auto i1 = v.end();
   auto i2 = i1;
   --i1;
-  *i2; // expected-warning{{Iterator accessed outside of its range}}
+  *i2; // expected-warning{{Past-the-end iterator dereferenced}}
 }
 
 void copy_and_increase1(const std::vector ) {
@@ -86,7 +65,7 @@
   auto i2 = i1;
   ++i1;
   if (i2 == v.end())
-*i2; // expected-warning{{Iterator accessed outside of its range}}
+*i2; // expected-warning{{Past-the-end iterator dereferenced}}
 }
 
 void copy_and_increase3(const std::vector ) {
@@ -94,7 +73,7 @@
   auto i2 = i1;
   ++i1;
   if (v.end() == i2)
-*i2; // expected-warning{{Iterator accessed outside of its range}}
+*i2; // expected-warning{{Past-the-end iterator dereferenced}}
 }
 
 template 
@@ -116,14 +95,14 @@
 
 void bad_non_std_find(std::vector , int e) {
   auto first = nonStdFind(V.begin(), V.end(), e);
-  *first; // expected-warning{{Iterator accessed outside of its range}}
+  *first; // expected-warning{{Past-the-end iterator dereferenced}}
 }
 
 void tricky(std::vector , int e) {
   const auto first = V.begin();
   const auto comp1 = (first != V.end()), comp2 = (first == V.end());
   if (comp1)
-*first;
+*first; // no-warning
 }
 
 void loop(std::vector , int e) {
@@ -147,7 +126,7 @@
   auto i0 = --L.cend();
   L.push_back(n);
   ++i0;
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{Past-the-end iterator dereferenced}}
 }
 
 void good_pop_back(std::list , int n) {
@@ -159,7 +138,7 @@
 void bad_pop_back(std::list , int n) {
   auto i0 = --L.cend(); --i0;
   L.pop_back();
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{Past-the-end iterator dereferenced}}
 }
 
 void good_push_front(std::list , int n) {
@@ -172,7 +151,7 @@
   auto i0 = L.cbegin();
   L.push_front(n);
   --i0;
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator decremented ahead of its valid range}}
 }
 
 void good_pop_front(std::list , int n) {
@@ -184,13 +163,13 @@
 void bad_pop_front(std::list , int n) {
   auto i0 = ++L.cbegin();
   L.pop_front();
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator decremented ahead of its valid range}}
 }
 
 void bad_move(std::list , std::list ) {
   auto i0 = --L2.cend();
   L1 = std::move(L2);
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{Past-the-end iterator dereferenced}}
 }
 
 void bad_move_push_back(std::list , std::list , int n) {
@@ -198,5 +177,25 @@
   L2.push_back(n);
   L1 = std::move(L2);
   ++i0;
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{Past-the-end iterator dereferenced}}
+}
+
+void good_incr_begin(const std::list ) {
+  auto i0 = L.begin();
+  ++i0; // no-warning
+}
+
+void bad_decr_begin(const std::list ) {
+  auto i0 = L.begin();
+  --i0;  // expected-warning{{Iterator decremented ahead of its valid range}}
+}
+
+void good_decr_end(const std::list ) {
+  auto i0 = L.end();
+  --i0; // no-warning
+}
+
+void bad_incr_end(const std::list ) {
+  auto i0 = L.end();
+  ++i0;  // expected-warning{{Iterator incremented behind the past-the-end iterator}}
 }
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ 

[PATCH] D54961: [AArch64] Add command-line option for SSBS

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348142: [AArch64] Add command-line option for SSBS (authored 
by pabbar01, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D54961?vs=175537=176388#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D54961

Files:
  test/Driver/aarch64-ssbs.c


Index: test/Driver/aarch64-ssbs.c
===
--- test/Driver/aarch64-ssbs.c
+++ test/Driver/aarch64-ssbs.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8a+ssbs   %s 
2>&1 | FileCheck %s
+// CHECK: "-target-feature" "+ssbs"
+
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8a+nossbs %s 
2>&1 | FileCheck %s --check-prefix=NOSSBS
+// NOSSBS: "-target-feature" "-ssbs"
+
+// RUN: %clang -### -target aarch64-none-none-eabi  %s 
2>&1 | FileCheck %s --check-prefix=ABSENTSSBS
+// ABSENTSSBS-NOT: "-target-feature" "+ssbs"
+// ABSENTSSBS-NOT: "-target-feature" "-ssbs"


Index: test/Driver/aarch64-ssbs.c
===
--- test/Driver/aarch64-ssbs.c
+++ test/Driver/aarch64-ssbs.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8a+ssbs   %s 2>&1 | FileCheck %s
+// CHECK: "-target-feature" "+ssbs"
+
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8a+nossbs %s 2>&1 | FileCheck %s --check-prefix=NOSSBS
+// NOSSBS: "-target-feature" "-ssbs"
+
+// RUN: %clang -### -target aarch64-none-none-eabi  %s 2>&1 | FileCheck %s --check-prefix=ABSENTSSBS
+// ABSENTSSBS-NOT: "-target-feature" "+ssbs"
+// ABSENTSSBS-NOT: "-target-feature" "-ssbs"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348142 - [AArch64] Add command-line option for SSBS

[Pablo Barrio via cfe-commits]

Author: pabbar01
Date: Mon Dec  3 06:40:37 2018
New Revision: 348142

URL: http://llvm.org/viewvc/llvm-project?rev=348142=rev
Log:
[AArch64] Add command-line option for SSBS

Summary:
SSBS (Speculative Store Bypass Safe) is only mandatory from 8.5
onwards but is optional from Armv8.0-A. This patch adds testing for
the ssbs command line option, added to allow enabling the feature
in previous Armv8-A architectures to 8.5.

Reviewers: olista01, samparker, aemerson

Reviewed By: samparker

Subscribers: javed.absar, kristof.beyls, cfe-commits

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

Added:
cfe/trunk/test/Driver/aarch64-ssbs.c

Added: cfe/trunk/test/Driver/aarch64-ssbs.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-ssbs.c?rev=348142=auto
==
--- cfe/trunk/test/Driver/aarch64-ssbs.c (added)
+++ cfe/trunk/test/Driver/aarch64-ssbs.c Mon Dec  3 06:40:37 2018
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8a+ssbs   %s 
2>&1 | FileCheck %s
+// CHECK: "-target-feature" "+ssbs"
+
+// RUN: %clang -### -target aarch64-none-none-eabi -march=armv8a+nossbs %s 
2>&1 | FileCheck %s --check-prefix=NOSSBS
+// NOSSBS: "-target-feature" "-ssbs"
+
+// RUN: %clang -### -target aarch64-none-none-eabi  %s 
2>&1 | FileCheck %s --check-prefix=ABSENTSSBS
+// ABSENTSSBS-NOT: "-target-feature" "+ssbs"
+// ABSENTSSBS-NOT: "-target-feature" "-ssbs"


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

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

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



Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+

aaron.ballman wrote:
> aaron.ballman wrote:
> > Running your test through GCC looks like the behavior matches here for C; 
> > can you also add a C++ test that demonstrates the behavior does not change?
> > 
> > https://godbolt.org/z/zRYDMG
> Strangely, the above godbolt link dropped the output windows, here's a 
> different link that shows the behavioral differences between C and C++ mode 
> in GCC: https://godbolt.org/z/R3zRHe
Hmm, I'll have a look at this.


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

https://reviews.llvm.org/D51211



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

[PATCH] D54903: [Sema] Improve static_assert diagnostics.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaTemplate.cpp:3055
 
+// Print a diagnostic for the failing static_assert expression. Defaults to
+// pretty-printing the expression.

courbet wrote:
> aaron.ballman wrote:
> > Comment is a bit out of date as this is no longer specific to 
> > `static_assert`.
> > 
> > It looks like this may also change the behavior of enable_if diagnostic 
> > reporting. Do you need to update any of those tests from this change? If 
> > not, can you devise some tests for that case as well to show that this 
> > isn't just a static_assert behavior? (Do a search for 
> > `findFailedBooleanCondition` to find where the behavior has changed.)
> Nope, unfortunately the code that calls this is actually untested...
> I could not find a test that actually enters the:
> ```
> if (TypeAliasTemplateDecl *AliasTemplate =
>   dyn_cast(Template))
> ```
> in `Sema::CheckTemplateIdType`.
> 
> I stopped at the following insanity:
> 
> ```
> // RUN: %clang_cc1 -std=c++11 -verify %s
> 
> template 
> struct is_same {
>   enum { value = 0 };
> };
> 
> template  struct is_same {
>   enum { value = 1 };
> };
> 
> struct Dispatch {
>   template  using SameAs = is_same;
> };
> 
> template 
> struct S {
>   template  using same = typename DispatchT::template SameAs;
> 
>   template 
>   static void foo() __attribute__((enable_if(same::value, "")));
> 
> };
> 
> void runFoo() {
>   // S::foo();
>   S::foo();
> 
> }
> ```
> 
> This one exercises the code up to `if (CanonType.isNull()) {`, but I'm not 
> sure how to get a null type here.
> 
How about this:
```
namespace boost {
  template struct enable_if {};
  template struct enable_if { typedef T type; };
}

template struct NonTemplateFunction {
  typename boost::enable_if::type f();
};

template 
struct Bobble {
  using type = T;
};

struct Frobble {
  using type = char;
};
NonTemplateFunction::type> NTFC;
```
That should trigger the path in `Sema::CheckTypenameType()`, I believe.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54903



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

[PATCH] D32950: Support C++1z features in `__has_extension`

[Filipe Cabecinhas via Phabricator via cfe-commits]

filcab added a comment.

Hi Eric,

I know this is old, but are you interested in reviving this patch? I don't know 
enough about clang's extensions to LGTM such a patch (updated for the current 
code), but would really like to have a way to know if extensions are supported.
We just now had people ask how to detect if `if constexpr` is available, and 
not having a feature check makes it much harder to figure out (and makes our 
test based on current clang behaviour).

Thank you,
Filipe


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

https://reviews.llvm.org/D32950



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

[PATCH] D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch

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

Szelethus added a comment.

Also, `AnalyzerOptions.def` was recently clan-formatted, feel free to run it 
again after the changes you make in it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55135



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

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:7711-7715
+  // It's an integer promotion if the destination type is the promoted
+  // source type.
+  return ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To;

ebevhan wrote:
> aaron.ballman wrote:
> > This check is not checking against the promoted type of the bit-field. See 
> > `Sema::UsualArithmeticConversions()` for an example of what I'm talking 
> > about. Is that expected?
> I'm not entirely sure what you mean. Are you referring to the type produced 
> by `isPromotableBitField`? The source type of that promotion is what we don't 
> want to see in these implicit casts.
> 
> We don't want to look through promotions here if we promoted from a type 
> which was the result of a bitfield promotion, and that bitfield promotion was 
> from a higher ranked type to a lower ranked type. so, if we have a bitfield 
> of type `short`, then promoting that to `int` is fine, and we will give a 
> warning for the `short`. But if the type of the bitfield is `long`, it could 
> be promoted to `int`. However, the format specifier warning will look through 
> these promotions and think that we passed an expression of `long` to the 
> function even though it was `int`.
Ahhh, okay, I see what's going on then. This makes more sense to me now, thank 
you for the explanation.



Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+

aaron.ballman wrote:
> Running your test through GCC looks like the behavior matches here for C; can 
> you also add a C++ test that demonstrates the behavior does not change?
> 
> https://godbolt.org/z/zRYDMG
Strangely, the above godbolt link dropped the output windows, here's a 
different link that shows the behavioral differences between C and C++ mode in 
GCC: https://godbolt.org/z/R3zRHe


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

https://reviews.llvm.org/D51211



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

[PATCH] D55188: Extract TextChildDumper class

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/ASTDumperUtils.h:22
+
+namespace ast_dumper {
+// Colors used for various parts of the AST dump

steveire wrote:
> aaron.ballman wrote:
> > I'm not certain this namespace is useful, especially when it gets imported 
> > at TU scope in ASTDumper.cpp.
> > it gets imported at TU scope in ASTDumper.cpp
> 
> Today that's the only place it is used. In the future that won't be true.
Then we can add the namespace in the future when we find a situation where 
we're worried about collisions? As it stands, this only provides the illusion 
of safety. If you really want to keep it, please pull the global using 
declaration.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55188



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

[PATCH] D55212: Handle alloc_size attribute on function pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/Attr.td:1072
 def AllocSize : InheritableAttr {
   let Spellings = [GCC<"alloc_size">];
+  let Subjects = SubjectList<[HasFunctionProto]>;

Does GCC support writing `alloc_size` on function pointers?



Comment at: lib/Sema/SemaDecl.cpp:6269
+static void copyAttrFromTypedefToDecl(Sema , Decl *D, const TypedefType *TT) 
{
+  TypedefNameDecl *TND = TT->getDecl();
+  if (AttrTy *Attribute = TND->getAttr()) {

`const TypedefNameDecl *` ?



Comment at: lib/Sema/SemaDecl.cpp:6270
+  TypedefNameDecl *TND = TT->getDecl();
+  if (AttrTy *Attribute = TND->getAttr()) {
+AttrTy *Clone = Attribute->clone(S.Context);

`const auto *`



Comment at: lib/Sema/SemaDecl.cpp:6723
+  if (R->isFunctionPointerType()) {
+if (auto TT = R->getAs()) {
+  copyAttrFromTypedefToDecl(*this, NewVD, TT);

`const auto *` and can elide the braces.

This should probably be generalized at some point into something declarative in 
Attr.td, though the type checking may be hard to do that for (I'm not certain). 
It doesn't need to be done as part of this patch, though.



Comment at: test/Sema/alloc-size.c:46
+// This typedef applies the alloc_size to the pointer to the function pointer 
and should not be allowed
+void *(**__attribute__((alloc_size(1, 2))) * allocator_function_typdef4)(int, 
int); // expected-warning{{'alloc_size' attribute only applies to non-K 
functions}}

What should happen in these situations?
```
typedef void * (__attribute__((alloc_size(1))) * 
my_malloc_fn_pointer_type)(int);
typedef void * (* my_other_malloc_fn_pointer_type)(int);

void *fn(int i);
__attribute__((alloc_size(1))) void *fn2(int i);

int main() {
  my_malloc_fn_pointer_type f = fn; // ???
  my_other_malloc_fn_pointer_type f2 = fn2; // ???
}
```
Should this code do something special?
```
typedef void * (__attribute__((alloc_size(1))) * 
my_malloc_fn_pointer_type)(int);
typedef void * (* my_other_malloc_fn_pointer_type)(int);

void overloaded(my_malloc_fn_pointer_type fn);
void overloaded(my_other_malloc_fn_pointer_type fn);

void *fn(int i);
__attribute__((alloc_size(1))) void *fn2(int i);

int main() {
  overloaded(fn);
  overloaded(fn2);
}

```


Repository:
  rC Clang

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

https://reviews.llvm.org/D55212



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

[PATCH] D55188: Extract TextChildDumper class

[Stephen Kelly via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/ASTDumperUtils.h:22
+
+namespace ast_dumper {
+// Colors used for various parts of the AST dump

aaron.ballman wrote:
> I'm not certain this namespace is useful, especially when it gets imported at 
> TU scope in ASTDumper.cpp.
> it gets imported at TU scope in ASTDumper.cpp

Today that's the only place it is used. In the future that won't be true.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55188



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

r348139 - [OpenCL][Sema] Improving formatting

[Marco Antognini via cfe-commits]

Author: mantognini
Date: Mon Dec  3 06:03:49 2018
New Revision: 348139

URL: http://llvm.org/viewvc/llvm-project?rev=348139=rev
Log:
[OpenCL][Sema] Improving formatting

Reformat comment added in r348120 following
review https://reviews.llvm.org/D55136.


Modified:
cfe/trunk/lib/Sema/SemaExpr.cpp

Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=348139=348138=348139=diff
==
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Mon Dec  3 06:03:49 2018
@@ -5567,9 +5567,8 @@ Sema::BuildResolvedCallExpr(Expr *Fn, Na
   Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {
 // Extract the return type from the (builtin) function pointer type.
 // FIXME Several builtins still have setType in
-//   Sema::CheckBuiltinFunctionCall. One should review their
-//   definitions in Builtins.def to ensure they are correct before
-//   removing setType calls.
+// Sema::CheckBuiltinFunctionCall. One should review their definitions in
+// Builtins.def to ensure they are correct before removing setType calls.
 QualType FnPtrTy = Context.getPointerType(FDecl->getType());
 Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();
 ResultTy = FDecl->getCallResultType();


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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> leonardchan wrote:
> > rjmccall wrote:
> > > ebevhan wrote:
> > > > rjmccall wrote:
> > > > > Hmm.  So adding a signed integer to an unsigned fixed-point type 
> > > > > always converts the integer to unsigned?  That's a little weird, but 
> > > > > only because the fixed-point rule seems to be the other way.  Anyway, 
> > > > > I assume it's not a bug in the spec.
> > > > `handleFixedPointConversion` only calculates the result type of the 
> > > > expression, not the calculation type. The final result type of the 
> > > > operation will be the unsigned fixed-point type, but the calculation 
> > > > itself will be done in a signed type with enough precision to represent 
> > > > both the signed integer and the unsigned fixed-point type. 
> > > > 
> > > > Though, if the result ends up being negative, the final result is 
> > > > undefined unless the type is saturating. I don't think there is a test 
> > > > for the saturating case (signed integer + unsigned saturating 
> > > > fixed-point) in the SaturatedAddition tests.
> > > > `handleFixedPointConversion` only calculates the result type of the 
> > > > expression, not the calculation type.
> > > 
> > > Right, I understand that, but the result type of the expression obviously 
> > > impacts the expressive range of the result, since you can end up with a 
> > > negative value.
> > > 
> > > Hmm.  With that said, if the general principle is to perform the 
> > > operation with full precision on the original operand values, why are 
> > > unsigned fixed-point operands coerced to their corresponding signed types 
> > > *before* the operation?  This is precision-limiting if the unsigned 
> > > representation doesn't use a padding bit.  That seems like a bug in the 
> > > spec.
> > > Hmm. With that said, if the general principle is to perform the operation 
> > > with full precision on the original operand values, why are unsigned 
> > > fixed-point operands coerced to their corresponding signed types *before* 
> > > the operation? This is precision-limiting if the unsigned representation 
> > > doesn't use a padding bit. That seems like a bug in the spec.
> > 
> > Possibly. When the standard mentions converting from signed to unsigned 
> > fixed point, the only requirement involved is conservation of magnitude 
> > (the number of integral bits excluding the sign)
> > 
> > ```
> > when signed and unsigned fixed-point types are mixed, the unsigned type is 
> > converted to the corresponding signed type, and this should go without loss 
> > of magnitude
> > ```
> > 
> > This is just speculation, but I'm under the impression that not as much 
> > "attention" was given for unsigned types as for signed types since "`In the 
> > embedded processor world, support for unsigned fixed-point data types is 
> > rare; normally only signed fixed-point data types are supported`", but was 
> > kept anyway since unsigned types are used a lot.
> > 
> > ```
> > However, to disallow unsigned fixed-point arithmetic from programming 
> > languages (in general, and from C in particular) based on this observation, 
> > seems overly restrictive.
> > ```
> > 
> > I also imagine that if the programmer needs more precision, the correct 
> > approach would be to cast up to a type with a higher scale. The standard 
> > seems to make an effort to expose as much in regards to the underlying 
> > fixed point types as possible:
> > 
> > ```
> > it should be possible to write fixed-point algorithms that are independent 
> > of the actual fixed-point hardware support. This implies that a programmer 
> > (or a running program) should have access to all parameters that define the 
> > behavior of the underlying hardware (in other words: even if these 
> > parameters are implementation-defined).
> > ```
> > 
> > So the user would at least know that unsigned types may not have the same 
> > scale as their corresponding signed types if the hardware handles them with 
> > different scales.
> > 
> > Also added test for signed integer + unsigned saturating fixed point.
> As long as we maintain the same typing behavior, does the standard permit us 
> to just Do The Right Thing here and do the extended arithmetic with the 
> original unsigned operand?  I'm sure there are some cases where we would 
> produce a slightly different value than an implementation that does the 
> coercion before the operation, but that might be permitted under the 
> standard, and as you say, it would only affect some situations that it 
> doesn't seem the standard has given much thought to.
The coercion of unsigned to signed is likely done for pragmatic reasons; if you 
have a signed and unsigned type of the same rank, operating on them together 
won't require any 'extra bits'. This means that if your hardware has native 
support for the operations, you won't end up in a 

[PATCH] D55139: [clangd] Avoid memory-mapping files on Windows

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 176382.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

- s/VolatileFSProvider/VolatileFileSystem


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55139

Files:
  clangd/CMakeLists.txt
  clangd/FSProvider.cpp
  clangd/FSProvider.h

Index: clangd/FSProvider.h
===
--- clangd/FSProvider.h
+++ clangd/FSProvider.h
@@ -33,9 +33,7 @@
 public:
   // FIXME: returns the single real FS instance, which is not threadsafe.
   llvm::IntrusiveRefCntPtr
-  getFileSystem() const override {
-return llvm::vfs::getRealFileSystem();
-  }
+  getFileSystem() const override;
 };
 
 } // namespace clangd
Index: clangd/FSProvider.cpp
===
--- /dev/null
+++ clangd/FSProvider.cpp
@@ -0,0 +1,85 @@
+//===--- FSProvider.cpp - VFS provider for ClangdServer ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FSProvider.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include 
+
+using namespace llvm;
+
+namespace clang {
+namespace clangd {
+
+namespace {
+/// Always opens files in the underlying filesystem as "volatile", meaning they
+/// won't be memory-mapped. This avoid locking the files on Windows.
+class VolatileFileSystem : public llvm::vfs::ProxyFileSystem {
+public:
+  explicit VolatileFileSystem(llvm::IntrusiveRefCntPtr FS)
+  : ProxyFileSystem(std::move(FS)) {}
+
+  llvm::ErrorOr>
+  openFileForRead(const Twine ) override {
+SmallString<128> Path;
+InPath.toVector(Path);
+
+auto File = getUnderlyingFS().openFileForRead(Path);
+if (!File)
+  return File;
+// Try to guess preamble files, they can be memory-mapped even on Windows as
+// clangd has exclusive access to those.
+StringRef FileName = llvm::sys::path::filename(Path);
+if (FileName.startswith("preamble-") && FileName.endswith(".pch"))
+  return File;
+return std::unique_ptr(
+new VolatileFile(std::move(*File)));
+  }
+
+private:
+  class VolatileFile : public vfs::File {
+  public:
+VolatileFile(std::unique_ptr Wrapped)
+: Wrapped(std::move(Wrapped)) {
+  assert(this->Wrapped);
+}
+
+virtual llvm::ErrorOr>
+getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
+  bool /*IsVolatile*/) override {
+  return Wrapped->getBuffer(Name, FileSize, RequiresNullTerminator,
+/*IsVolatile=*/true);
+}
+
+llvm::ErrorOr status() override { return Wrapped->status(); }
+llvm::ErrorOr getName() override { return Wrapped->getName(); }
+std::error_code close() override { return Wrapped->close(); }
+
+  private:
+std::unique_ptr Wrapped;
+  };
+};
+} // namespace
+
+llvm::IntrusiveRefCntPtr
+clang::clangd::RealFileSystemProvider::getFileSystem() const {
+// Avoid using memory-mapped files on Windows, they cause file locking issues.
+// FIXME: Try to use a similar approach in Sema instead of relying on
+//error-prone propagation of the 'isVolatile' flag through all layers.
+#ifdef _WIN32
+  return new VolatileFSProvider(vfs::getRealFileSystem());
+#else
+  return vfs::getRealFileSystem();
+#endif
+}
+} // namespace clangd
+} // namespace clang
Index: clangd/CMakeLists.txt
===
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -23,6 +23,7 @@
   FindSymbols.cpp
   FileDistance.cpp
   FS.cpp
+  FSProvider.cpp
   FuzzyMatch.cpp
   GlobalCompilationDatabase.cpp
   Headers.cpp
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55139: [clangd] Avoid memory-mapping files on Windows

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 176381.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

- Keep using memory-mapped files for PCHs
- Move once


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55139

Files:
  clangd/CMakeLists.txt
  clangd/FSProvider.cpp
  clangd/FSProvider.h

Index: clangd/FSProvider.h
===
--- clangd/FSProvider.h
+++ clangd/FSProvider.h
@@ -33,9 +33,7 @@
 public:
   // FIXME: returns the single real FS instance, which is not threadsafe.
   llvm::IntrusiveRefCntPtr
-  getFileSystem() const override {
-return llvm::vfs::getRealFileSystem();
-  }
+  getFileSystem() const override;
 };
 
 } // namespace clangd
Index: clangd/FSProvider.cpp
===
--- /dev/null
+++ clangd/FSProvider.cpp
@@ -0,0 +1,85 @@
+//===--- FSProvider.cpp - VFS provider for ClangdServer ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FSProvider.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include 
+
+using namespace llvm;
+
+namespace clang {
+namespace clangd {
+
+namespace {
+/// Always opens files in the underlying filesystem as "volatile", meaning they
+/// won't be memory-mapped. This avoid locking the files on Windows.
+class VolatileFSProvider : public llvm::vfs::ProxyFileSystem {
+public:
+  explicit VolatileFSProvider(llvm::IntrusiveRefCntPtr FS)
+  : ProxyFileSystem(std::move(FS)) {}
+
+  llvm::ErrorOr>
+  openFileForRead(const Twine ) override {
+SmallString<128> Path;
+InPath.toVector(Path);
+
+auto File = getUnderlyingFS().openFileForRead(Path);
+if (!File)
+  return File;
+// Try to guess preamble files, they can be memory-mapped even on Windows as
+// clangd has exclusive access to those.
+StringRef FileName = llvm::sys::path::filename(Path);
+if (FileName.startswith("preamble-") && FileName.endswith(".pch"))
+  return File;
+return std::unique_ptr(
+new VolatileFile(std::move(*File)));
+  }
+
+private:
+  class VolatileFile : public vfs::File {
+  public:
+VolatileFile(std::unique_ptr Wrapped)
+: Wrapped(std::move(Wrapped)) {
+  assert(this->Wrapped);
+}
+
+virtual llvm::ErrorOr>
+getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
+  bool /*IsVolatile*/) override {
+  return Wrapped->getBuffer(Name, FileSize, RequiresNullTerminator,
+/*IsVolatile=*/true);
+}
+
+llvm::ErrorOr status() override { return Wrapped->status(); }
+llvm::ErrorOr getName() override { return Wrapped->getName(); }
+std::error_code close() override { return Wrapped->close(); }
+
+  private:
+std::unique_ptr Wrapped;
+  };
+};
+} // namespace
+
+llvm::IntrusiveRefCntPtr
+clang::clangd::RealFileSystemProvider::getFileSystem() const {
+// Avoid using memory-mapped files on Windows, they cause file locking issues.
+// FIXME: Try to use a similar approach in Sema instead of relying on
+//error-prone propagation of the 'isVolatile' flag through all layers.
+#ifdef _WIN32
+  return new VolatileFSProvider(vfs::getRealFileSystem());
+#else
+  return vfs::getRealFileSystem();
+#endif
+}
+} // namespace clangd
+} // namespace clang
Index: clangd/CMakeLists.txt
===
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -23,6 +23,7 @@
   FindSymbols.cpp
   FileDistance.cpp
   FS.cpp
+  FSProvider.cpp
   FuzzyMatch.cpp
   GlobalCompilationDatabase.cpp
   Headers.cpp
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55139: [clangd] Avoid memory-mapping files on Windows

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clangd/FSProvider.cpp:33
+  return File;
+return std::unique_ptr(
+new VolatileFile(std::move(std::move(*File;

kadircet wrote:
> make_unique?
Unfortunately it doesn't work here - `VolatileFile` is private in this class.



Comment at: clangd/FSProvider.cpp:34
+return std::unique_ptr(
+new VolatileFile(std::move(std::move(*File;
+  }

kadircet wrote:
> can we move once ?
We need moar moves!!! xD

Thanks for noticing, fixed.




Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55139



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

[PATCH] D54903: [Sema] Improve static_assert diagnostics.

[Clement Courbet via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaTemplate.cpp:3055
 
+// Print a diagnostic for the failing static_assert expression. Defaults to
+// pretty-printing the expression.

aaron.ballman wrote:
> Comment is a bit out of date as this is no longer specific to `static_assert`.
> 
> It looks like this may also change the behavior of enable_if diagnostic 
> reporting. Do you need to update any of those tests from this change? If not, 
> can you devise some tests for that case as well to show that this isn't just 
> a static_assert behavior? (Do a search for `findFailedBooleanCondition` to 
> find where the behavior has changed.)
Nope, unfortunately the code that calls this is actually untested...
I could not find a test that actually enters the:
```
if (TypeAliasTemplateDecl *AliasTemplate =
  dyn_cast(Template))
```
in `Sema::CheckTemplateIdType`.

I stopped at the following insanity:

```
// RUN: %clang_cc1 -std=c++11 -verify %s

template 
struct is_same {
  enum { value = 0 };
};

template  struct is_same {
  enum { value = 1 };
};

struct Dispatch {
  template  using SameAs = is_same;
};

template 
struct S {
  template  using same = typename DispatchT::template SameAs;

  template 
  static void foo() __attribute__((enable_if(same::value, "")));

};

void runFoo() {
  // S::foo();
  S::foo();

}
```

This one exercises the code up to `if (CanonType.isNull()) {`, but I'm not sure 
how to get a null type here.



Repository:
  rC Clang

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

https://reviews.llvm.org/D54903



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

[PATCH] D55124: [CodeComplete] Cleanup access checking in code completion

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348135: [CodeComplete] Cleanup access checking in code 
completion (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55124

Files:
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Parse/ParseExprCXX.cpp
  cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp
  cfe/trunk/lib/Sema/SemaAccess.cpp
  cfe/trunk/lib/Sema/SemaCodeComplete.cpp
  cfe/trunk/test/CodeCompletion/accessibility-crash.cpp
  cfe/trunk/test/CodeCompletion/accessibility.cpp

Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -6065,7 +6065,8 @@
 bool ForceCheck = false,
 bool ForceUnprivileged = false);
   void CheckLookupAccess(const LookupResult );
-  bool IsSimplyAccessible(NamedDecl *decl, DeclContext *Ctx);
+  bool IsSimplyAccessible(NamedDecl *Decl, CXXRecordDecl *NamingClass,
+  QualType BaseType);
   bool isSpecialMemberAccessibleForDeletion(CXXMethodDecl *decl,
 AccessSpecifier access,
 QualType objectType);
@@ -10340,7 +10341,7 @@
   void CodeCompleteAssignmentRHS(Scope *S, Expr *LHS);
 
   void CodeCompleteQualifiedId(Scope *S, CXXScopeSpec ,
-   bool EnteringContext);
+   bool EnteringContext, QualType BaseType);
   void CodeCompleteUsing(Scope *S);
   void CodeCompleteUsingDirective(Scope *S);
   void CodeCompleteNamespaceDecl(Scope *S);
Index: cfe/trunk/test/CodeCompletion/accessibility.cpp
===
--- cfe/trunk/test/CodeCompletion/accessibility.cpp
+++ cfe/trunk/test/CodeCompletion/accessibility.cpp
@@ -0,0 +1,73 @@
+class X {
+public:
+ int pub;
+protected:
+ int prot;
+private:
+ int priv;
+};
+
+class Unrelated {
+public:
+  static int pub;
+protected:
+  static int prot;
+private:
+  static int priv;
+};
+
+class Y : public X {
+  int test() {
+this->pub = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:11 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS %s
+// THIS: priv (InBase,Inaccessible)
+// THIS: prot (InBase)
+// THIS: pub (InBase)
+//
+// Also check implicit 'this->', i.e. complete at the start of the line.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:1 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS %s
+
+X().pub + 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:32:9 %s -o - \
+// RUN: | FileCheck -check-prefix=X-OBJ %s
+// X-OBJ: priv (Inaccessible)
+// X-OBJ: prot (Inaccessible)
+// X-OBJ: pub : [#int#]pub
+
+Y().pub + 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:39:9 %s -o - \
+// RUN: | FileCheck -check-prefix=Y-OBJ %s
+// Y-OBJ: priv (InBase,Inaccessible)
+// Y-OBJ: prot (InBase)
+// Y-OBJ: pub (InBase)
+
+this->X::pub = 10;
+X::pub = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:46:14 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS-BASE %s
+//
+// THIS-BASE: priv (Inaccessible)
+// THIS-BASE: prot : [#int#]prot
+// THIS-BASE: pub : [#int#]pub
+//
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:47:8 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS-BASE %s
+
+
+this->Unrelated::pub = 10; // a check we don't crash in this cases.
+Y().Unrelated::pub = 10; // a check we don't crash in this cases.
+Unrelated::pub = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:59:22 %s -o - \
+// RUN: | FileCheck -check-prefix=UNRELATED %s
+// UNRELATED: priv (Inaccessible)
+// UNRELATED: prot (Inaccessible)
+// UNRELATED: pub : [#int#]pub
+//
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:60:20 %s -o - \
+// RUN: | FileCheck -check-prefix=UNRELATED %s
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:61:16 %s -o - \
+// RUN: | FileCheck -check-prefix=UNRELATED %s
+  }
+};
Index: cfe/trunk/test/CodeCompletion/accessibility-crash.cpp
===
--- cfe/trunk/test/CodeCompletion/accessibility-crash.cpp
+++ cfe/trunk/test/CodeCompletion/accessibility-crash.cpp
@@ -0,0 +1,23 @@
+class X {
+public:
+ int pub;
+protected:
+ int prot;
+private:
+ int priv;
+};
+
+class Y : public X {
+  int test() {
+[]() {
+
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:1 %s -o - \
+  // RUN: | FileCheck %s
+  // CHECK: priv (InBase,Inaccessible)
+  // CHECK: prot (InBase)
+  

r348135 - [CodeComplete] Cleanup access checking in code completion

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Mon Dec  3 05:29:17 2018
New Revision: 348135

URL: http://llvm.org/viewvc/llvm-project?rev=348135=rev
Log:
[CodeComplete] Cleanup access checking in code completion

Summary: Also fixes a crash (see the added 'accessibility-crash.cpp' test).

Reviewers: ioeric, kadircet

Reviewed By: kadircet

Subscribers: cfe-commits

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

Added:
cfe/trunk/test/CodeCompletion/accessibility-crash.cpp
cfe/trunk/test/CodeCompletion/accessibility.cpp
Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Parse/ParseExprCXX.cpp
cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp
cfe/trunk/lib/Sema/SemaAccess.cpp
cfe/trunk/lib/Sema/SemaCodeComplete.cpp

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=348135=348134=348135=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Dec  3 05:29:17 2018
@@ -6065,7 +6065,8 @@ public:
 bool ForceCheck = false,
 bool ForceUnprivileged = false);
   void CheckLookupAccess(const LookupResult );
-  bool IsSimplyAccessible(NamedDecl *decl, DeclContext *Ctx);
+  bool IsSimplyAccessible(NamedDecl *Decl, CXXRecordDecl *NamingClass,
+  QualType BaseType);
   bool isSpecialMemberAccessibleForDeletion(CXXMethodDecl *decl,
 AccessSpecifier access,
 QualType objectType);
@@ -10340,7 +10341,7 @@ public:
   void CodeCompleteAssignmentRHS(Scope *S, Expr *LHS);
 
   void CodeCompleteQualifiedId(Scope *S, CXXScopeSpec ,
-   bool EnteringContext);
+   bool EnteringContext, QualType BaseType);
   void CodeCompleteUsing(Scope *S);
   void CodeCompleteUsingDirective(Scope *S);
   void CodeCompleteNamespaceDecl(Scope *S);

Modified: cfe/trunk/lib/Parse/ParseExprCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseExprCXX.cpp?rev=348135=348134=348135=diff
==
--- cfe/trunk/lib/Parse/ParseExprCXX.cpp (original)
+++ cfe/trunk/lib/Parse/ParseExprCXX.cpp Mon Dec  3 05:29:17 2018
@@ -235,22 +235,11 @@ bool Parser::ParseOptionalCXXScopeSpecif
 
   while (true) {
 if (HasScopeSpecifier) {
-  // C++ [basic.lookup.classref]p5:
-  //   If the qualified-id has the form
-  //
-  //   ::class-name-or-namespace-name::...
-  //
-  //   the class-name-or-namespace-name is looked up in global scope as a
-  //   class-name or namespace-name.
-  //
-  // To implement this, we clear out the object type as soon as we've
-  // seen a leading '::' or part of a nested-name-specifier.
-  ObjectType = nullptr;
-
   if (Tok.is(tok::code_completion)) {
 // Code completion for a nested-name-specifier, where the code
 // completion token follows the '::'.
-Actions.CodeCompleteQualifiedId(getCurScope(), SS, EnteringContext);
+Actions.CodeCompleteQualifiedId(getCurScope(), SS, EnteringContext,
+ObjectType.get());
 // Include code completion token into the range of the scope otherwise
 // when we try to annotate the scope tokens the dangling code 
completion
 // token will cause assertion in
@@ -259,6 +248,18 @@ bool Parser::ParseOptionalCXXScopeSpecif
 cutOffParsing();
 return true;
   }
+
+  // C++ [basic.lookup.classref]p5:
+  //   If the qualified-id has the form
+  //
+  //   ::class-name-or-namespace-name::...
+  //
+  //   the class-name-or-namespace-name is looked up in global scope as a
+  //   class-name or namespace-name.
+  //
+  // To implement this, we clear out the object type as soon as we've
+  // seen a leading '::' or part of a nested-name-specifier.
+  ObjectType = nullptr;
 }
 
 // nested-name-specifier:

Modified: cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp?rev=348135=348134=348135=diff
==
--- cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp (original)
+++ cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp Mon Dec  3 05:29:17 2018
@@ -555,6 +555,9 @@ void PrintingCodeCompleteConsumer::Proce
   Tags.push_back("Hidden");
 if (Results[I].InBaseClass)
   Tags.push_back("InBase");
+if (Results[I].Availability ==
+CXAvailabilityKind::CXAvailability_NotAccessible)
+  Tags.push_back("Inaccessible");
 if (!Tags.empty())
   OS << " (" << llvm::join(Tags, ",") << 

[PATCH] D55191: [clangd] Refine the way of checking a declaration is referenced by the written code.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The **idea** of using the AST-based approach here was really nice, it was less 
expensive and seemed to clearly look at the semantic.
I wonder if there's a way to keep it on the AST level, without looking at the 
source locations.

What are the cases we're trying to filter out? Only implicit constructor or 
anything else?




Comment at: unittests/clangd/XRefsTests.cpp:1228
 
+TEST(FindReferences, ExplicitSymbols) {
+  const char *Tests[] = {

I'm missing what does this test actually tests.
The absence of implicit references (I guess constructor expressions)?



Comment at: unittests/clangd/XRefsTests.cpp:1259
+  namespace ns {
+  using [fo^o];
+  }

Shouldn't the `usings` always be qualified? Isn't this a compiler error?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55191



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

[PATCH] D54900: [Sema] Avoid CallExpr::setNumArgs in Sema::BuildCallToObjectOfClassType

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348134: [Sema] Avoid CallExpr::setNumArgs in 
Sema::BuildCallToObjectOfClassType (authored by brunoricci, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54900?vs=175431=176375#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54900

Files:
  cfe/trunk/lib/Sema/SemaOverload.cpp


Index: cfe/trunk/lib/Sema/SemaOverload.cpp
===
--- cfe/trunk/lib/Sema/SemaOverload.cpp
+++ cfe/trunk/lib/Sema/SemaOverload.cpp
@@ -13257,29 +13257,14 @@
   if (NewFn.isInvalid())
 return true;
 
+  // The number of argument slots to allocate in the call. If we have default
+  // arguments we need to allocate space for them as well. We additionally
+  // need one more slot for the object parameter.
+  unsigned NumArgsSlots = 1 + std::max(Args.size(), NumParams);
+
   // Build the full argument list for the method call (the implicit object
   // parameter is placed at the beginning of the list).
-  SmallVector MethodArgs(Args.size() + 1);
-  MethodArgs[0] = Object.get();
-  std::copy(Args.begin(), Args.end(), MethodArgs.begin() + 1);
-
-  // Once we've built TheCall, all of the expressions are properly
-  // owned.
-  QualType ResultTy = Method->getReturnType();
-  ExprValueKind VK = Expr::getValueKindForType(ResultTy);
-  ResultTy = ResultTy.getNonLValueExprType(Context);
-
-  CXXOperatorCallExpr *TheCall = new (Context)
-  CXXOperatorCallExpr(Context, OO_Call, NewFn.get(), MethodArgs, ResultTy,
-  VK, RParenLoc, FPOptions());
-
-  if (CheckCallReturnType(Method->getReturnType(), LParenLoc, TheCall, Method))
-return true;
-
-  // We may have default arguments. If so, we need to allocate more
-  // slots in the call for them.
-  if (Args.size() < NumParams)
-TheCall->setNumArgs(Context, NumParams + 1);
+  SmallVector MethodArgs(NumArgsSlots);
 
   bool IsError = false;
 
@@ -13291,7 +13276,7 @@
 IsError = true;
   else
 Object = ObjRes;
-  TheCall->setArg(0, Object.get());
+  MethodArgs[0] = Object.get();
 
   // Check the argument types.
   for (unsigned i = 0; i != NumParams; i++) {
@@ -13320,7 +13305,7 @@
   Arg = DefArg.getAs();
 }
 
-TheCall->setArg(i + 1, Arg);
+MethodArgs[i + 1] = Arg;
   }
 
   // If this is a variadic call, handle args passed through "...".
@@ -13330,14 +13315,27 @@
   ExprResult Arg = DefaultVariadicArgumentPromotion(Args[i], 
VariadicMethod,
 nullptr);
   IsError |= Arg.isInvalid();
-  TheCall->setArg(i + 1, Arg.get());
+  MethodArgs[i + 1] = Arg.get();
 }
   }
 
-  if (IsError) return true;
+  if (IsError)
+return true;
 
   DiagnoseSentinelCalls(Method, LParenLoc, Args);
 
+  // Once we've built TheCall, all of the expressions are properly owned.
+  QualType ResultTy = Method->getReturnType();
+  ExprValueKind VK = Expr::getValueKindForType(ResultTy);
+  ResultTy = ResultTy.getNonLValueExprType(Context);
+
+  CXXOperatorCallExpr *TheCall = new (Context)
+  CXXOperatorCallExpr(Context, OO_Call, NewFn.get(), MethodArgs, ResultTy,
+  VK, RParenLoc, FPOptions());
+
+  if (CheckCallReturnType(Method->getReturnType(), LParenLoc, TheCall, Method))
+return true;
+
   if (CheckFunctionCall(Method, TheCall, Proto))
 return true;
 


Index: cfe/trunk/lib/Sema/SemaOverload.cpp
===
--- cfe/trunk/lib/Sema/SemaOverload.cpp
+++ cfe/trunk/lib/Sema/SemaOverload.cpp
@@ -13257,29 +13257,14 @@
   if (NewFn.isInvalid())
 return true;
 
+  // The number of argument slots to allocate in the call. If we have default
+  // arguments we need to allocate space for them as well. We additionally
+  // need one more slot for the object parameter.
+  unsigned NumArgsSlots = 1 + std::max(Args.size(), NumParams);
+
   // Build the full argument list for the method call (the implicit object
   // parameter is placed at the beginning of the list).
-  SmallVector MethodArgs(Args.size() + 1);
-  MethodArgs[0] = Object.get();
-  std::copy(Args.begin(), Args.end(), MethodArgs.begin() + 1);
-
-  // Once we've built TheCall, all of the expressions are properly
-  // owned.
-  QualType ResultTy = Method->getReturnType();
-  ExprValueKind VK = Expr::getValueKindForType(ResultTy);
-  ResultTy = ResultTy.getNonLValueExprType(Context);
-
-  CXXOperatorCallExpr *TheCall = new (Context)
-  CXXOperatorCallExpr(Context, OO_Call, NewFn.get(), MethodArgs, ResultTy,
-  VK, RParenLoc, FPOptions());
-
-  if (CheckCallReturnType(Method->getReturnType(), LParenLoc, TheCall, Method))
-return true;
-
-  // We may have default arguments. If so, we need to allocate more
-  // slots in the call for them.

r348134 - [Sema] Avoid CallExpr::setNumArgs in Sema::BuildCallToObjectOfClassType

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Mon Dec  3 05:23:56 2018
New Revision: 348134

URL: http://llvm.org/viewvc/llvm-project?rev=348134=rev
Log:
[Sema] Avoid CallExpr::setNumArgs in Sema::BuildCallToObjectOfClassType

CallExpr::setNumArgs is the only thing that prevents storing the arguments
of a call expression in a trailing array since it might resize the argument
array. setNumArgs is only called in 3 places in Sema, and for all of them it
is possible to avoid it.

This deals with the call to setNumArgs in BuildCallToObjectOfClassType.
Instead of constructing the CXXOperatorCallExpr first and later calling
setNumArgs if we have default arguments, we first construct a large
enough SmallVector, do the promotion/check of the arguments, and
then construct the CXXOperatorCallExpr.

Incidentally this also avoid reallocating the arguments when the
call operator has default arguments but this is not the primary goal.

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

Reviewed By: aaron.ballman


Modified:
cfe/trunk/lib/Sema/SemaOverload.cpp

Modified: cfe/trunk/lib/Sema/SemaOverload.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=348134=348133=348134=diff
==
--- cfe/trunk/lib/Sema/SemaOverload.cpp (original)
+++ cfe/trunk/lib/Sema/SemaOverload.cpp Mon Dec  3 05:23:56 2018
@@ -13257,29 +13257,14 @@ Sema::BuildCallToObjectOfClassType(Scope
   if (NewFn.isInvalid())
 return true;
 
+  // The number of argument slots to allocate in the call. If we have default
+  // arguments we need to allocate space for them as well. We additionally
+  // need one more slot for the object parameter.
+  unsigned NumArgsSlots = 1 + std::max(Args.size(), NumParams);
+
   // Build the full argument list for the method call (the implicit object
   // parameter is placed at the beginning of the list).
-  SmallVector MethodArgs(Args.size() + 1);
-  MethodArgs[0] = Object.get();
-  std::copy(Args.begin(), Args.end(), MethodArgs.begin() + 1);
-
-  // Once we've built TheCall, all of the expressions are properly
-  // owned.
-  QualType ResultTy = Method->getReturnType();
-  ExprValueKind VK = Expr::getValueKindForType(ResultTy);
-  ResultTy = ResultTy.getNonLValueExprType(Context);
-
-  CXXOperatorCallExpr *TheCall = new (Context)
-  CXXOperatorCallExpr(Context, OO_Call, NewFn.get(), MethodArgs, ResultTy,
-  VK, RParenLoc, FPOptions());
-
-  if (CheckCallReturnType(Method->getReturnType(), LParenLoc, TheCall, Method))
-return true;
-
-  // We may have default arguments. If so, we need to allocate more
-  // slots in the call for them.
-  if (Args.size() < NumParams)
-TheCall->setNumArgs(Context, NumParams + 1);
+  SmallVector MethodArgs(NumArgsSlots);
 
   bool IsError = false;
 
@@ -13291,7 +13276,7 @@ Sema::BuildCallToObjectOfClassType(Scope
 IsError = true;
   else
 Object = ObjRes;
-  TheCall->setArg(0, Object.get());
+  MethodArgs[0] = Object.get();
 
   // Check the argument types.
   for (unsigned i = 0; i != NumParams; i++) {
@@ -13320,7 +13305,7 @@ Sema::BuildCallToObjectOfClassType(Scope
   Arg = DefArg.getAs();
 }
 
-TheCall->setArg(i + 1, Arg);
+MethodArgs[i + 1] = Arg;
   }
 
   // If this is a variadic call, handle args passed through "...".
@@ -13330,14 +13315,27 @@ Sema::BuildCallToObjectOfClassType(Scope
   ExprResult Arg = DefaultVariadicArgumentPromotion(Args[i], 
VariadicMethod,
 nullptr);
   IsError |= Arg.isInvalid();
-  TheCall->setArg(i + 1, Arg.get());
+  MethodArgs[i + 1] = Arg.get();
 }
   }
 
-  if (IsError) return true;
+  if (IsError)
+return true;
 
   DiagnoseSentinelCalls(Method, LParenLoc, Args);
 
+  // Once we've built TheCall, all of the expressions are properly owned.
+  QualType ResultTy = Method->getReturnType();
+  ExprValueKind VK = Expr::getValueKindForType(ResultTy);
+  ResultTy = ResultTy.getNonLValueExprType(Context);
+
+  CXXOperatorCallExpr *TheCall = new (Context)
+  CXXOperatorCallExpr(Context, OO_Call, NewFn.get(), MethodArgs, ResultTy,
+  VK, RParenLoc, FPOptions());
+
+  if (CheckCallReturnType(Method->getReturnType(), LParenLoc, TheCall, Method))
+return true;
+
   if (CheckFunctionCall(Method, TheCall, Proto))
 return true;
 


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

[PATCH] D55188: Extract TextChildDumper class

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/ASTDumperUtils.h:22
+
+namespace ast_dumper {
+// Colors used for various parts of the AST dump

I'm not certain this namespace is useful, especially when it gets imported at 
TU scope in ASTDumper.cpp.



Comment at: include/clang/AST/ASTDumperUtils.h:96
+
+struct TextChildDumper {
+  raw_ostream 

I'm not sold on the name for this class. It's a bit too generic to understand 
what it does. How about `ASTDumpLayoutFormatter` (and `ASTDumpNodeFormatter` 
for the node dumper)?



Comment at: include/clang/AST/ASTDumperUtils.h:97-110
+  raw_ostream 
+  const bool ShowColors;
+
+  /// Pending[i] is an action to dump an entity at level i.
+  llvm::SmallVector, 32> Pending;
+
+  /// Indicates whether we're at the top level.

I don't think these members should be public.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55188



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

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaChecking.cpp:7711-7715
+  // It's an integer promotion if the destination type is the promoted
+  // source type.
+  return ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To;

aaron.ballman wrote:
> This check is not checking against the promoted type of the bit-field. See 
> `Sema::UsualArithmeticConversions()` for an example of what I'm talking 
> about. Is that expected?
I'm not entirely sure what you mean. Are you referring to the type produced by 
`isPromotableBitField`? The source type of that promotion is what we don't want 
to see in these implicit casts.

We don't want to look through promotions here if we promoted from a type which 
was the result of a bitfield promotion, and that bitfield promotion was from a 
higher ranked type to a lower ranked type. so, if we have a bitfield of type 
`short`, then promoting that to `int` is fine, and we will give a warning for 
the `short`. But if the type of the bitfield is `long`, it could be promoted to 
`int`. However, the format specifier warning will look through these promotions 
and think that we passed an expression of `long` to the function even though it 
was `int`.


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

https://reviews.llvm.org/D51211



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