[PATCH] D54466: [Analyzer] [WIP] Iterator Checkers - Use the base region of C++ Base Object Regions (recursively) for iterators stored in a region

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

baloghadamsoftware created this revision.
baloghadamsoftware added a reviewer: NoQ.
baloghadamsoftware added a project: clang.
Herald added subscribers: donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, 
szepet.
Herald added a reviewer: george.karpenkov.

Some operations, especially iterator comparisons are done on a subregion of the 
original region. Currently, iterator checker does not recognize these 
comparisons as iterator comparisons since the key for the abstract iterator 
position in the GDM is the base region, not the subregion. In other parts of 
the iterator checker we faced similar problems for containers which are always 
represented as regions. There the solution was to recursively retrieve the base 
region of `CXXBaseObjectRegion` classes. We apply the same solution now for 
iterators represented as regions. To avoid code repetition we moved this 
operation into a separate function.


Repository:
  rC Clang

https://reviews.llvm.org/D54466

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp

Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -390,6 +390,7 @@
const MemRegion *Reg);
 bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos);
 bool isZero(ProgramStateRef State, const NonLoc &Val);
+const MemRegion *getBaseForCXXBase(const MemRegion *Reg);
 } // namespace
 
 IteratorChecker::IteratorChecker() {
@@ -1089,9 +1090,7 @@
 void IteratorChecker::verifyMatch(CheckerContext &C, const SVal &Iter,
   const MemRegion *Cont) const {
   // Verify match between a container and the container of an iterator
-  while (const auto *CBOR = Cont->getAs()) {
-Cont = CBOR->getSuperRegion();
-  }
+  Cont = getBaseForCXXBase(Cont);
 
   auto State = C.getState();
   const auto *Pos = getIteratorPosition(State, Iter);
@@ -1125,9 +1124,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = getBaseForCXXBase(ContReg);
 
   // If the container already has a begin symbol then use it. Otherwise first
   // create a new one.
@@ -1151,9 +1148,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = getBaseForCXXBase(ContReg);
 
   // If the container already has an end symbol then use it. Otherwise first
   // create a new one.
@@ -1174,9 +1169,7 @@
 void IteratorChecker::assignToContainer(CheckerContext &C, const Expr *CE,
 const SVal &RetVal,
 const MemRegion *Cont) const {
-  while (const auto *CBOR = Cont->getAs()) {
-Cont = CBOR->getSuperRegion();
-  }
+  Cont = getBaseForCXXBase(Cont);
 
   auto State = C.getState();
   auto &SymMgr = C.getSymbolManager();
@@ -1194,9 +1187,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = getBaseForCXXBase(ContReg);
 
   // Assignment of a new value to a container always invalidates all its
   // iterators
@@ -1211,9 +1202,7 @@
   if (!OldCont.isUndef()) {
 const auto *OldContReg = OldCont.getAsRegion();
 if (OldContReg) {
-  while (const auto *CBOR = OldContReg->getAs()) {
-OldContReg = CBOR->getSuperRegion();
-  }
+  OldContReg = getBaseForCXXBase(OldContReg);
   const auto OldCData = getContainerData(State, OldContReg);
   if (OldCData) {
 if (const auto OldEndSym = OldCData->getEnd()) {
@@ -1273,9 +1262,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = getBaseForCXXBase(ContReg);
 
   // The clear() operation invalidates all the iterators, except the past-end
   // iterators of list-like containers
@@ -1302,9 +1289,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = getBaseForCXXBase(ContReg);
 
   // For deque-like containers invalidate all iterator positions
   auto State = C.getState();
@@ -1341,9 +1326,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = getBaseForCXXBase(ContReg);
 
   auto State = C.getState();
   const auto CData = getContainerData(State, ContReg);
@@ -1381,9 +1364,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = getBaseForCXXBase(ContReg);
 
   // For deque-like containers invalidate all iterator positions
   auto State = C.getState();
@@ -1416,9 +1397,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs(

[PATCH] D54466: [Analyzer] [WIP] Iterator Checkers - Use the base region of C++ Base Object Regions (recursively) for iterators stored in a region

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

baloghadamsoftware added a comment.

I marked this patch as WIP because I could not create a test-case for it. 
However in real projects this patch seems to reduce false positives 
significantly.

The member function `getBaseRegion()` of `MemRegion` does not work here because 
it recursively retrieves the base region of multiple kinds of `SubRegion`.


Repository:
  rC Clang

https://reviews.llvm.org/D54466



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

r346748 - UserManual: Tweak the /Zc:dllexportInlines- docs some

[Hans Wennborg via cfe-commits]

Author: hans
Date: Tue Nov 13 01:05:12 2018
New Revision: 346748

URL: http://llvm.org/viewvc/llvm-project?rev=346748&view=rev
Log:
UserManual: Tweak the /Zc:dllexportInlines- docs some

Addressing comments on https://reviews.llvm.org/D54319

Modified:
cfe/trunk/docs/UsersManual.rst
cfe/trunk/include/clang/Driver/CLCompatOptions.td

Modified: cfe/trunk/docs/UsersManual.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=346748&r1=346747&r2=346748&view=diff
==
--- cfe/trunk/docs/UsersManual.rst (original)
+++ cfe/trunk/docs/UsersManual.rst Tue Nov 13 01:05:12 2018
@@ -2947,8 +2947,8 @@ Execute ``clang-cl /?`` to see a list of
   /Yc   Generate a pch file for all code up to and 
including 
   /Yu   Load a pch file and use it instead of all code 
up to and including 
   /Z7 Enable CodeView debug information in object files
-  /Zc:dllexportInlines-   Don't dllexport/import inline member functions 
of dllexport/import classes
-  /Zc:dllexportInlinesdllexport/import inline member functions of 
dllexport/import classes (default)
+  /Zc:dllexportInlines-   Don't dllexport/dllimport inline member 
functions of dllexport/import classes
+  /Zc:dllexportInlinesdllexport/dllimport inline member functions of 
dllexport/import classes (default)
   /Zc:sizedDealloc-   Disable C++14 sized global deallocation functions
   /Zc:sizedDeallocEnable C++14 sized global deallocation functions
   /Zc:strictStrings   Treat string literals as const
@@ -3101,10 +3101,10 @@ line.
 The /Zc:dllexportInlines- Option
 
 
-This causes the class-level `dllexport` and `dllimport` attributes not to be
-applied to inline member functions, as they otherwise would. For example, in
-the code below `S::foo()` would normally be defined and exported by the DLL,
-but when using the ``/Zc:dllexportInlines-`` flag it is not:
+This causes the class-level `dllexport` and `dllimport` attributes to not apply
+to inline member functions, as they otherwise would. For example, in the code
+below `S::foo()` would normally be defined and exported by the DLL, but when
+using the ``/Zc:dllexportInlines-`` flag it is not:
 
 .. code-block:: c
 
@@ -3170,7 +3170,8 @@ different instance of that variable than
   }
 
 This could lead to very subtle bugs. Using ``-fvisibility-inlines-hidden`` can
-lead to the same issue.
+lead to the same issue. To avoid it in this case, make `S::foo()` or
+`internal()` non-inline, or mark them `dllimport/dllexport` explicitly.
 
 The /fallback Option
 

Modified: cfe/trunk/include/clang/Driver/CLCompatOptions.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CLCompatOptions.td?rev=346748&r1=346747&r2=346748&view=diff
==
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td (original)
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td Tue Nov 13 01:05:12 2018
@@ -336,9 +336,9 @@ def _SLASH_Yu : CLJoined<"Yu">,
 def _SLASH_Y_ : CLFlag<"Y-">,
   HelpText<"Disable precompiled headers, overrides /Yc and /Yu">;
 def _SLASH_Zc_dllexportInlines : CLFlag<"Zc:dllexportInlines">,
-  HelpText<"dllexport/import inline member functions of dllexport/import 
classes (default)">;
+  HelpText<"dllexport/dllimport inline member functions of dllexport/import 
classes (default)">;
 def _SLASH_Zc_dllexportInlines_ : CLFlag<"Zc:dllexportInlines-">,
-  HelpText<"Don't dllexport/import inline member functions of dllexport/import 
classes">;
+  HelpText<"Don't dllexport/dllimport inline member functions of 
dllexport/import classes">;
 def _SLASH_Fp : CLJoined<"Fp">,
   HelpText<"Set pch filename (with /Yc and /Yu)">, MetaVarName<"">;
 


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

[PATCH] D54319: clang-cl: Add documentation for /Zc:dllexportInlines-

[Hans Wennborg via Phabricator via cfe-commits]

hans marked 3 inline comments as done.
hans added a comment.

Thanks for the review! I made adjustments in r346748.




Comment at: cfe/trunk/docs/UsersManual.rst:3104
+
+This causes the class-level `dllexport` and `dllimport` attributes not to be
+applied to inline member functions, as they otherwise would. For example, in

thakis wrote:
> "to not be applied" sounds more correct to me, but I'm not a native speaker 
> either. And I think "to not apply to inline member functions" works to and is 
> shorter and not passive, and it makes the part after the comma sound better 
> (with the passive I think it might have to be "as they otherwise would be"?)
Yeah, "to not apply to inline member functions" is better.


Repository:
  rL LLVM

https://reviews.llvm.org/D54319



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

[PATCH] D54453: Remove myself as owner of clang-query.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

Thanks Aaron for volunteering, I'm very thankful for all your work on the 
reviews, it's much appreciated :D


Repository:
  rL LLVM

https://reviews.llvm.org/D54453



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

[PATCH] D54405: Record whether a AST Matcher constructs a Node

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 173819.
steveire added a comment.

Update


Repository:
  rC Clang

https://reviews.llvm.org/D54405

Files:
  include/clang/ASTMatchers/ASTMatchersInternal.h
  lib/ASTMatchers/Dynamic/Registry.cpp


Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -44,18 +44,54 @@
 
 using ConstructorMap = llvm::StringMap>;
 
+using NodeConstructorMap =
+std::map>;
+
 class RegistryMaps {
 public:
   RegistryMaps();
   ~RegistryMaps();
 
   const ConstructorMap &constructors() const { return Constructors; }
+  const NodeConstructorMap &nodeConstructors() const {
+return NodeConstructors;
+  }
+
+  void registerNodeMatcher(ast_type_traits::ASTNodeKind kind,
+   internal::MatcherDescriptor *Callback);
 
 private:
   void registerMatcher(StringRef MatcherName,
std::unique_ptr Callback);
 
+  template 
+  void registerIfNodeMatcher(MatcherType,
+ internal::MatcherDescriptor *Descriptor,
+ StringRef) {}
+
+  template 
+  void registerIfNodeMatcher(
+  ast_matchers::internal::VariadicAllOfMatcher VarFunc,
+  internal::MatcherDescriptor *Descriptor, StringRef MatcherName) {
+auto NodeKind = ast_type_traits::ASTNodeKind::getFromNodeKind<
+typename 
ast_matchers::internal::VariadicAllOfMatcher::Type>();
+if (!NodeKind.isNone())
+  NodeConstructors[NodeKind] = std::make_pair(MatcherName, Descriptor);
+  }
+
+  template 
+  void registerIfNodeMatcher(
+  ast_matchers::internal::VariadicDynCastAllOfMatcher
+  VarFunc,
+  internal::MatcherDescriptor *Descriptor, StringRef MatcherName) {
+auto NodeKind = ast_type_traits::ASTNodeKind::getFromNodeKind();
+if (!NodeKind.isNone())
+  NodeConstructors[NodeKind] = std::make_pair(MatcherName, Descriptor);
+  }
+
   ConstructorMap Constructors;
+  NodeConstructorMap NodeConstructors;
 };
 
 } // namespace
@@ -67,8 +103,13 @@
 }
 
 #define REGISTER_MATCHER(name) 
\
-  registerMatcher(#name, internal::makeMatcherAutoMarshall(
\
- ::clang::ast_matchers::name, #name));
+  {
\
+auto Descriptor =  
\
+internal::makeMatcherAutoMarshall(::clang::ast_matchers::name, #name); 
\
+registerIfNodeMatcher(::clang::ast_matchers::name, Descriptor.get(),   
\
+  #name);  
\
+registerMatcher(#name, std::move(Descriptor)); 
\
+  }
 
 #define REGISTER_MATCHER_OVERLOAD(name)
\
   registerMatcher(#name,   
\
Index: include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- include/clang/ASTMatchers/ASTMatchersInternal.h
+++ include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -1577,6 +1577,7 @@
 : public VariadicFunction, Matcher,
   makeAllOfComposite> {
 public:
+  using Type = T;
   VariadicAllOfMatcher() {}
 };
 


Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -44,18 +44,54 @@
 
 using ConstructorMap = llvm::StringMap>;
 
+using NodeConstructorMap =
+std::map>;
+
 class RegistryMaps {
 public:
   RegistryMaps();
   ~RegistryMaps();
 
   const ConstructorMap &constructors() const { return Constructors; }
+  const NodeConstructorMap &nodeConstructors() const {
+return NodeConstructors;
+  }
+
+  void registerNodeMatcher(ast_type_traits::ASTNodeKind kind,
+   internal::MatcherDescriptor *Callback);
 
 private:
   void registerMatcher(StringRef MatcherName,
std::unique_ptr Callback);
 
+  template 
+  void registerIfNodeMatcher(MatcherType,
+ internal::MatcherDescriptor *Descriptor,
+ StringRef) {}
+
+  template 
+  void registerIfNodeMatcher(
+  ast_matchers::internal::VariadicAllOfMatcher VarFunc,
+  internal::MatcherDescriptor *Descriptor, StringRef MatcherName) {
+auto NodeKind = ast_type_traits::ASTNodeKind::getFromNodeKind<
+typename ast_matchers::internal::VariadicAllOfMatcher::Type>();
+if (!NodeKind.isNone())
+  NodeConstructors[NodeKind] = std::make_pair(MatcherName, Descriptor);
+  }
+
+  template 
+  void registerIfNodeMatcher(
+  ast_matchers::internal::VariadicDynCastAllOfMatcher
+  VarFunc,
+  internal::MatcherDescriptor *Descriptor

[PATCH] D54402: Extract method to allow re-use

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 173820.
steveire added a comment.

Update


Repository:
  rC Clang

https://reviews.llvm.org/D54402

Files:
  lib/ASTMatchers/Dynamic/Registry.cpp

Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -561,9 +561,9 @@
   return std::vector(TypeSet.begin(), TypeSet.end());
 }
 
-std::vector
-Registry::getMatcherCompletions(ArrayRef AcceptedTypes) {
-  std::vector Completions;
+template 
+void processAcceptableMatchers(ArrayRef AcceptedTypes,
+   Callable &&Func) {
 
   // Search the registry for acceptable matchers.
   for (const auto &M : RegistryData->constructors()) {
@@ -593,49 +593,66 @@
 }
 
 if (!RetKinds.empty() && MaxSpecificity > 0) {
-  std::string Decl;
-  llvm::raw_string_ostream OS(Decl);
-
-  if (IsPolymorphic) {
-OS << "Matcher " << Name << "(Matcher";
-  } else {
-OS << "Matcher<" << RetKinds << "> " << Name << "(";
-for (const std::vector &Arg : ArgsKinds) {
-  if (&Arg != &ArgsKinds[0])
-OS << ", ";
-
-  bool FirstArgKind = true;
-  std::set MatcherKinds;
-  // Two steps. First all non-matchers, then matchers only.
-  for (const ArgKind &AK : Arg) {
-if (AK.getArgKind() == ArgKind::AK_Matcher) {
-  MatcherKinds.insert(AK.getMatcherKind());
-} else {
-  if (!FirstArgKind) OS << "|";
-  FirstArgKind = false;
-  OS << AK.asString();
+  std::forward(Func)(Name, Matcher, RetKinds, ArgsKinds,
+   MaxSpecificity);
+}
+  }
+}
+
+std::vector
+Registry::getMatcherCompletions(ArrayRef AcceptedTypes) {
+  std::vector Completions;
+
+  processAcceptableMatchers(
+  AcceptedTypes,
+  [&Completions](StringRef Name, const MatcherDescriptor &Matcher,
+ std::set &RetKinds,
+ std::vector> ArgsKinds,
+ unsigned MaxSpecificity) {
+std::string Decl;
+llvm::raw_string_ostream OS(Decl);
+
+if (Matcher.isPolymorphic()) {
+  OS << "Matcher " << Name << "(Matcher";
+} else {
+  OS << "Matcher<" << RetKinds << "> " << Name << "(";
+  for (const std::vector &Arg : ArgsKinds) {
+if (&Arg != &ArgsKinds[0])
+  OS << ", ";
+
+bool FirstArgKind = true;
+std::set MatcherKinds;
+// Two steps. First all non-matchers, then matchers only.
+for (const ArgKind &AK : Arg) {
+  if (AK.getArgKind() == ArgKind::AK_Matcher) {
+MatcherKinds.insert(AK.getMatcherKind());
+  } else {
+if (!FirstArgKind)
+  OS << "|";
+FirstArgKind = false;
+OS << AK.asString();
+  }
+}
+if (!MatcherKinds.empty()) {
+  if (!FirstArgKind)
+OS << "|";
+  OS << "Matcher<" << MatcherKinds << ">";
 }
-  }
-  if (!MatcherKinds.empty()) {
-if (!FirstArgKind) OS << "|";
-OS << "Matcher<" << MatcherKinds << ">";
   }
 }
-  }
-  if (Matcher.isVariadic())
-OS << "...";
-  OS << ")";
-
-  std::string TypedText = Name;
-  TypedText += "(";
-  if (ArgsKinds.empty())
-TypedText += ")";
-  else if (ArgsKinds[0][0].getArgKind() == ArgKind::AK_String)
-TypedText += "\"";
-
-  Completions.emplace_back(TypedText, OS.str(), MaxSpecificity);
-}
-  }
+if (Matcher.isVariadic())
+  OS << "...";
+OS << ")";
+
+std::string TypedText = Name;
+TypedText += "(";
+if (ArgsKinds.empty())
+  TypedText += ")";
+else if (ArgsKinds[0][0].getArgKind() == ArgKind::AK_String)
+  TypedText += "\"";
+
+Completions.emplace_back(TypedText, OS.str(), MaxSpecificity);
+  });
 
   return Completions;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54404: Exclude matchers which can have multiple results

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Let's discuss it on IRC at some point and see if we can come up with wording.


Repository:
  rC Clang

https://reviews.llvm.org/D54404



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

[PATCH] D54405: Record whether a AST Matcher constructs a Node

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:77
+  internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) {
+auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<
+typename 
ast_matchers::internal::VariadicAllOfMatcher::Type>();

zturner wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > zturner wrote:
> > > > steveire wrote:
> > > > > aaron.ballman wrote:
> > > > > > steveire wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Mildly not keen on the use of `auto` here. It's a factory 
> > > > > > > > function, so it kind of names the resulting type, but it also 
> > > > > > > > looks like the type will be 
> > > > > > > > `ast_matchers::internal::VariadicAllOfMatcher::Type` 
> > > > > > > > from the template argument, which is incorrect.
> > > > > > > There is no reason to assume that taking a template argument 
> > > > > > > means that type is result.
> > > > > > > 
> > > > > > > The method is `getFrom` which decreases the ambiguity still 
> > > > > > > further.
> > > > > > > 
> > > > > > > Spelling out the type doesn't add anything useful. This should be 
> > > > > > > ok.
> > > > > > > There is no reason to assume that taking a template argument 
> > > > > > > means that type is result.
> > > > > > 
> > > > > > Aside from all the other places that do exactly that (getAs<>, 
> > > > > > cast<>, dyn_cast<>, castAs<>, and so on). Generally, when we have a 
> > > > > > function named get that takes a template type argument, the result 
> > > > > > when seen in proximity to auto is the template type.
> > > > > > 
> > > > > > > Spelling out the type doesn't add anything useful. This should be 
> > > > > > > ok.
> > > > > > 
> > > > > > I slept on this one and fall on the other side of it; using auto 
> > > > > > hides information that tripped up at least one person when reading 
> > > > > > the code, so don't use auto. It's not clear enough what the 
> > > > > > resulting type will be.
> > > > > I put this in the category of requiring 
> > > > > 
> > > > > SomeType ST = SomeType::create();
> > > > > 
> > > > > instead of 
> > > > > 
> > > > > auto ST = SomeType::create();
> > > > > 
> > > > > `ast_type_traits::ASTNodeKind` is already on that line and you want 
> > > > > to see it again.
> > > > > 
> > > > FWIW I'm with Aaron here.  Im' not familiar at all with this codebase, 
> > > > and looking at the code, my first instinct is "the result type is 
> > > > probably 
> > > > `ast_matchers::internal::VariadicAllOfMatcher::Type`".  
> > > > According to Aaron's earlier comment, that is incorrect, so there's at 
> > > > least 1 data point that it hinders readability.
> > > > 
> > > > So, honest question.  What *is* the return type here?
> > > > So, honest question. What *is* the return type here?
> > > 
> > > Much to my surprise, it's `ASTNodeKind`. It was entirely unobvious to me 
> > > that this was a factory function.
> > @zturner Quoting myself:
> > 
> > > `ast_type_traits::ASTNodeKind` is already on that line and you want to 
> > > see it again.
> > 
> > The expression is `getFromNodeKind`. There is a pattern of 
> > `SomeType::fromFooKind()` returning a `SomeType`. This is not so 
> > unusual.
> > 
> > 
> Note that at the top of this file there's already a `using namespace 
> clang::ast_type_traits;`  So if you're worried about verbosity, then you can 
> remove the `ast_type_traits::`, remove the `auto`, and the net effect is that 
> the overall line will end up being shorter.
The funny thing is - if you see a line like 

Parser CodeParser = Parser::getFromArgs(Args);

you have no idea what type `Parser` is! 

To start, it could be `clang::Parser` or 
`clang::ast_matchers::dynamic::Parser`, depending on a `using namespace` which 
might appear any distance up in the file. It is not uncommon for clang files to 
be thousands of lines lone.

Or it could be inside a template with `template`, or there 
could be a `using Parser = Foo;` any distance up in the file. 

Parser CodeParser = Parser::getFromArgs(Args);

is no more helpful than

auto CodeParser = Parser::getFromArgs(Args);

Sticking with the same example, if `CodeParser` is a field, then you might have 
a line

CodeParser = Parser::getFromArgs(Args);

and you could object and create a macro which expands to nothing to ensure that 
the type appears on the line

CodeParser = CALL_RETURNS(Parser)Parser::getFromArgs(Args);

No one does that, because it is ok for the type to not appear on the line.

You would also have to object to code such as 

Object.setParser(Parser::getFromArgs(Args));

requiring it to instead be

Parser CodeParser = Parser::getFromArgs(Args);
Object.setParser(CodeParser);

so that you can read the type.

Even then, what if those two lines are separated by a full page of code? How do 
you know the type of `CodeParser` in the `Object.setParser(CodeParser)` call? 
The answer is you don't

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

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

Szelethus added a comment.
Herald added a subscriber: baloghadamsoftware.

Allow me to disagree again -- the reason why `CheckerRegistry` and the entire 
checker registration process is a mess is that suboptimal solutions were added 
instead of making the system do well on the long term. This is completely fine, 
I am guilty of this myself, but now that this problem has been highlighted, 
it's time to get things right. Some clever refactoring is long overdue.

I don't mind delaying my checker option related works, if that means that 
checker registration doesn't need hacking each time a flaw like this is 
discovered.

There are other things I need to figure out -- some checkers refuse to register 
themselves in their registry funcions,

- which makes moving `CheckerManager::registerChecker` out of registry 
funcions impossible,
- which makes passing the checkers full name directly to `CheckerManager` 
impossible,
- which makes replacing the current registration infrastructure impossible as 
`CheckerManager` has to be mutable when supplied to the registry function,
- which will always leaves toom for errors like this to arise.

Since I spent the last couple weeks lookig at these files, there's no better 
opportunity for me to fix these.


Repository:
  rC Clang

https://reviews.llvm.org/D54438



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth added a comment.

CUDA maps `__shared__` internally also to `__attribute__((shared))`:

  #define __annotate__(a) \
  __attribute__((a))
  #define __location__(a) \
  __annotate__(a)
  ...
  #define __shared__ \
  __location__(shared)

My guess is that Clang does it just the same way and only converts to 
`LangAS::cuda_shared` for code generation in `GetGlobalVarAddressSpace`:
https://clang.llvm.org/doxygen/CodeGenModule_8cpp_source.html#l03305
In contrast, OpenCL uses keywords that are mapped directly to 
`LangAS::opencl_local` etc.


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54269: Introduce shard storage to auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 173824.
kadircet marked 18 inline comments as done.
kadircet added a comment.

- Address comments.
- Move loading from cache out of indexing.
- Use a factory for creation of index storage.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269

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

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -76,5 +76,81 @@
FileURI("unittest:///root/B.cc")}));
 }
 
+TEST(BackgroundIndexTest, ShardStorageTest) {
+  class MemoryShardStorage : public BackgroundIndexStorage {
+mutable std::mutex StorageMu;
+llvm::StringMap &Storage;
+size_t &CacheHits;
+
+  public:
+MemoryShardStorage(llvm::StringMap &Storage, size_t &CacheHits)
+: Storage(Storage), CacheHits(CacheHits) {}
+
+bool storeShard(llvm::StringRef ShardIdentifier,
+IndexFileOut Shard) const override {
+  std::lock_guard Lock(StorageMu);
+  std::string &str = Storage[ShardIdentifier];
+  llvm::raw_string_ostream OS(str);
+  OS << Shard;
+  OS.flush();
+  return true;
+}
+llvm::Expected
+retrieveShard(llvm::StringRef ShardIdentifier) const override {
+  std::lock_guard Lock(StorageMu);
+  if (Storage.find(ShardIdentifier) == Storage.end())
+return llvm::make_error(
+"Shard not found.", llvm::inconvertibleErrorCode());
+  auto IndexFile = readIndexFile(Storage[ShardIdentifier]);
+  if (!IndexFile)
+return IndexFile;
+  CacheHits++;
+  return IndexFile;
+}
+bool initialize(llvm::StringRef Directory) { return true; }
+  };
+  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;
+  BackgroundIndexStorage::setStorageFactory(
+  [&Storage, &CacheHits](llvm::StringRef) {
+return std::make_shared(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.
+  {
+BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"});
+Idx.enqueue(testPath("root"), Cmd);
+Idx.blockUntilIdleForTest();
+  }
+  EXPECT_EQ(CacheHits, 0U);
+  EXPECT_EQ(Storage.size(), 2U);
+  EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
+  EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
+
+  // Check A.cc has been loaded from cache.
+  {
+BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"});
+Idx.enqueue(testPath("root"), Cmd);
+Idx.blockUntilIdleForTest();
+  }
+  EXPECT_EQ(CacheHits, 1U);
+  EXPECT_EQ(Storage.size(), 2U);
+  EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
+  EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -14,6 +14,7 @@
 #include "FSProvider.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
+#include "index/Serialization.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/SHA1.h"
@@ -27,14 +28,45 @@
 namespace clang {
 namespace clangd {
 
+// Handles storage and retrieval of index shards.
+class BackgroundIndexStorage {
+private:
+  static std::function(llvm::StringRef)>
+  Factory;
+
+public:
+  using FileDigest = decltype(llvm::SHA1::hash({}));
+
+  // Stores given shard associationg with ShardIdentifier, which can be
+  // retrieved later on with the same identifier.
+  virtual bool storeShard(llvm::StringRef ShardIdentifier,
+  IndexFileOut Shard) const = 0;
+
+  // Retrieves the shard if found, also returns hash for the source file that
+  // generated this shard.
+  virtual llvm::Expected
+  retrieveShard(llvm::StringRef ShardIdentifier) const = 0;
+
+  template  static void setStorageFactory(T Factory) {
+BackgroundIndexStorage::Factory = Factory;
+  }
+
+  static std::shared_ptr
+  getForDirectory(llvm::StringRef Directory) {
+if (!Factory)
+  return nullptr;
+return Factory(Directory);
+  }
+};
+
 // Builds an in-memory index by by running the static indexer action over
 // all commands in a compilation database. Indexing happens in the background.
 // FIXME: it should also persist its state on disk for f

[PATCH] D54269: Introduce shard storage to auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/index/Background.h:39
+  retrieveShard(llvm::StringRef ShardIdentifier, FileDigest Hash) const = 0;
+  virtual bool initialize(llvm::StringRef Directory) = 0;
+};

kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > sammccall wrote:
> > > > Why not use the constructor? what does "directory" mean in the general 
> > > > case?
> > > Directory refers to the one specified in CompilationDatabase(which is 
> > > usually the build directory?), sorry for the inconvenience.
> > > I wasn't sure about where we plan to instantiate BackgroundIndex. If you 
> > > plan to do that initialization at a point in which we already know the 
> > > build directory we can move that to constructor, especially only to the 
> > > constructor of DiskBackedIndexStorage.
> > tooling::CompileCommand::WorkingDirectory? That doesn't seem especially 
> > relevant here.
> > Or the directory that the CDB was discovered in?
> > 
> > Yes, this seems to be only relevant to DiskBackedIndexStorage
> I suppose you meant `tooling::CompileCommand::Directory` rather than 
> `WorkingDirectory` ? That is the one I was talking about, why do you think it 
> is irrelevant ?
Changed logic to use Directory passed to enqueueAll or enqueue, which I assume 
is the directory that the compilation database lives. And I assumed we will 
have at most one compilation database per directory.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269



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

[PATCH] D40016: Use ImplicitConversionSequence::setAsIdentityConversion(QualType). NFC

[Alexander Richardson via Phabricator via cfe-commits]

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

rebased on latest HEAD


Repository:
  rC Clang

https://reviews.llvm.org/D40016

Files:
  lib/Sema/SemaOverload.cpp


Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -4780,10 +4780,7 @@
 InitializedEntity::InitializeParameter(S.Context, ToType,
/*Consumed=*/false);
   if (S.CanPerformCopyInitialization(Entity, From)) {
-Result.setStandard();
-Result.Standard.setAsIdentityConversion();
-Result.Standard.setFromType(ToType);
-Result.Standard.setAllToTypes(ToType);
+Result.setAsIdentityConversion(ToType);
 return Result;
   }
 }
@@ -4832,10 +4829,7 @@
 // For an empty list, we won't have computed any conversion sequence.
 // Introduce the identity conversion sequence.
 if (From->getNumInits() == 0) {
-  Result.setStandard();
-  Result.Standard.setAsIdentityConversion();
-  Result.Standard.setFromType(ToType);
-  Result.Standard.setAllToTypes(ToType);
+  Result.setAsIdentityConversion(ToType);
 }
 
 Result.setStdInitializerListElement(toStdInitializerList);
@@ -4973,10 +4967,7 @@
 //- if the initializer list has no elements, the implicit conversion
 //  sequence is the identity conversion.
 else if (NumInits == 0) {
-  Result.setStandard();
-  Result.Standard.setAsIdentityConversion();
-  Result.Standard.setFromType(ToType);
-  Result.Standard.setAllToTypes(ToType);
+  Result.setAsIdentityConversion(ToType);
 }
 return Result;
   }


Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -4780,10 +4780,7 @@
 InitializedEntity::InitializeParameter(S.Context, ToType,
/*Consumed=*/false);
   if (S.CanPerformCopyInitialization(Entity, From)) {
-Result.setStandard();
-Result.Standard.setAsIdentityConversion();
-Result.Standard.setFromType(ToType);
-Result.Standard.setAllToTypes(ToType);
+Result.setAsIdentityConversion(ToType);
 return Result;
   }
 }
@@ -4832,10 +4829,7 @@
 // For an empty list, we won't have computed any conversion sequence.
 // Introduce the identity conversion sequence.
 if (From->getNumInits() == 0) {
-  Result.setStandard();
-  Result.Standard.setAsIdentityConversion();
-  Result.Standard.setFromType(ToType);
-  Result.Standard.setAllToTypes(ToType);
+  Result.setAsIdentityConversion(ToType);
 }
 
 Result.setStdInitializerListElement(toStdInitializerList);
@@ -4973,10 +4967,7 @@
 //- if the initializer list has no elements, the implicit conversion
 //  sequence is the identity conversion.
 else if (NumInits == 0) {
-  Result.setStandard();
-  Result.Standard.setAsIdentityConversion();
-  Result.Standard.setFromType(ToType);
-  Result.Standard.setAllToTypes(ToType);
+  Result.setAsIdentityConversion(ToType);
 }
 return Result;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52273: [clangd] Initial implementation of expected types

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Forgot to say - the scope here looks just right, thanks for slimming this down!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52273



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

[PATCH] D52273: [clangd] Initial implementation of expected types

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/ExpectedTypes.cpp:12
+
+static llvm::Optional toEquivClass(ASTContext &Ctx, QualType T) {
+  if (T.isNull() || T->isDependentType())

returning QualType vs Type*? It seems we strip all qualifiers, seems clearest 
for the return type to reflect that.



Comment at: clangd/ExpectedTypes.cpp:15
+return llvm::None;
+  T = T.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+  if (T->isIntegerType() && !T->isEnumeralType())

Maybe we want Ctx.getUnqualifiedArrayType here or (more likely?) do 
array-to-pointer decay?



Comment at: clangd/ExpectedTypes.cpp:16
+  T = T.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+  if (T->isIntegerType() && !T->isEnumeralType())
+return QualType(Ctx.IntTy);

wow, "enumeral" might be my favorite c++-made-up word, displacing "emplace"...



Comment at: clangd/ExpectedTypes.cpp:25
+typeOfCompletion(const CodeCompletionResult &R) {
+  if (!R.Declaration)
+return llvm::None;

nit: dyn_cast_or_null below instead?



Comment at: clangd/ExpectedTypes.cpp:30
+return llvm::None;
+  QualType T = VD->getType().getCanonicalType().getNonReferenceType();
+  if (!T->isFunctionType())

nit: is canonicalization necessary here? you do it in toEquivClass
(I guess dropping references is, for the function type check)



Comment at: clangd/ExpectedTypes.cpp:33
+return T;
+  // Functions are a special case. They are completed as 'foo()' and we want to
+  // match their return type rather than the function type itself.

nit: I'd put the special case in the if() block, but up to you



Comment at: clangd/ExpectedTypes.cpp:37
+  // after the function name, e.g. `std::cout << std::endl`.
+  return T->getAs()->getReturnType().getNonReferenceType();
+}

dropping references seems redundant here, as you do it again later



Comment at: clangd/ExpectedTypes.cpp:46
+  llvm::SmallString<128> Out;
+  if (index::generateUSRForType(T, Ctx, Out))
+return llvm::None;

I think ultimately we may want to replace this with a custom walker:
 - we may want to ignore attributes (e.g. const) or bail out in some cases
 - generateUSRForType may not have the exact semantics we want for other random 
reasons
 - we can do tricks with hash_combine to avoid actually building huge strings 
we don't care about

not something for this patch, but maybe a FIXME?




Comment at: clangd/ExpectedTypes.cpp:71
+return llvm::None;
+  T = toEquivClass(Ctx, *T);
+  if (!T)

can you reuse fromPreferredType for the rest?



Comment at: clangd/ExpectedTypes.h:32
+/// this allows the representation to be stored in the index and compared with
+/// types coming from a different AST later.
+class OpaqueType {

Does this need to be a separate class rather than using `std::string`?
There are echoes of `SymbolID` here, but there were some factors that don't 
apply here:
 - it was fixed-width
 - memory layout was important as we stored lots of these in memory
 - we hashed them a lot and wanted a specific hash function

I suspect at least initially producing a somewhat readable std::string a la 
USRGeneration would be enough.



Comment at: unittests/clangd/ExpectedTypeTest.cpp:79
+ << (Matched ? "matched" : "did not match")
+ << "\n\tTarget type: " << To.getAsString()
+ << "\n\tSource value type: " << V->getType().getAsString();

note that if you think it's useful you can To.dump(*L->stream())
Maybe this is more interesting if/when we have a custom visitor.



Comment at: unittests/clangd/ExpectedTypeTest.cpp:92
+
+TEST_F(ExpectedTypeConversionTest, BasicTypes) {
+  build(R"cpp(

I really like the declarative equivalence-class setup of the tests.

A couple of suggestions:
 - maybe store the equivalence classes as groups of strings rather than decls, 
and lazily grab the decls. It's easier to tersely represent them...
 - I think the "convertibleTo" DSL obscures/abstracts the actual APIs you're 
testing - they build opaque types, and you're asserting equality.
 - pairwise assertion messages may not give enough context: if you expect a == 
b == c, and a != b, then whether a == c and b == c are probably relevant

I'd consider actually building up the equivalence classes `map>` and writing a `MATCHER_P2(ClassesAre, 
/*vector>*/Classes, /*ParsedAST*/AST, "classes are " + 
testing::PrintToString(Classes))`

That way the actual and expected equivalence classes will be dumped on failure, 
and you can still grab the decls/types from the AST to dump their details.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52273



_

[PATCH] D54453: Remove myself as owner of clang-query.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

> Thanks Aaron for volunteering, I'm very thankful for all your work on the 
> reviews, it's much appreciated :D

+1 ;)


Repository:
  rL LLVM

https://reviews.llvm.org/D54453



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

[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error

[Zixuan Wu via Phabricator via cfe-commits]

wuzish added inline comments.



Comment at: clang/test/CodeGen/altivec-generic-overload.c:74
+  convert1(gv1);
+  // CHECK: call void @_Z8convert1Dv16_a(<16 x i8> %{{[0-9]+}})
+  convert1(gv2);

hubert.reinterpretcast wrote:
> Checking that the call is to the expected target in terms of overload 
> resolution can be achieved by having a distinct return types for each member 
> of the overload set.
What's meaning of `having a distinct return types for each member of the 
overload set`?

Could you please give a example to show your expect?


https://reviews.llvm.org/D53417



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

[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

[Sander de Smalen via Phabricator via cfe-commits]

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



Comment at: include/clang/Basic/Attr.td:1792
+  let Spellings = [GNU<"aarch64_vector_pcs">,
+   CXX11<"clang", "aarch64_vector_pcs">,
+   Keyword<"__aarch64_vector_pcs">,

aaron.ballman wrote:
> Rather than using GNU and CXX11 spellings, you should use the Clang spelling. 
> If the attribute is not useful in C, then set `allowInC` to 0 instead of its 
> default of 1.
Thanks for the suggestion! The attribute is valid in C as well, and is tested 
for both C and C++ in test/CodeGen/aarch64-vpcs.c.



Comment at: include/clang/Basic/Attr.td:1794
+   Keyword<"__aarch64_vector_pcs">,
+   Keyword<"_aarch64_vector_pcs">];
+  let Documentation = [AArch64VectorPcsDocs];

aaron.ballman wrote:
> This steps on the user's namespace -- is that intended and necessary?
That was not intended and looking at the spec, the keyword is not required so 
I'll remove it.


https://reviews.llvm.org/D54425



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

[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen updated this revision to Diff 173831.
sdesmalen added a comment.

- Removed `_aarch64_vector_pcs` and `__aarch64_vector_pcs` keywords in favour 
of supporting only `__attribute__(aarch64_vector_pcs))`.


https://reviews.llvm.org/D54425

Files:
  include/clang-c/Index.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/Specifiers.h
  lib/AST/ItaniumMangle.cpp
  lib/AST/Type.cpp
  lib/AST/TypePrinter.cpp
  lib/Basic/Targets/AArch64.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaType.cpp
  test/CodeGen/aarch64-vpcs.c
  tools/libclang/CXType.cpp

Index: tools/libclang/CXType.cpp
===
--- tools/libclang/CXType.cpp
+++ tools/libclang/CXType.cpp
@@ -651,6 +651,7 @@
   TCALLINGCONV(X86Pascal);
   TCALLINGCONV(X86RegCall);
   TCALLINGCONV(X86VectorCall);
+  TCALLINGCONV(AArch64VectorCall);
   TCALLINGCONV(Win64);
   TCALLINGCONV(X86_64SysV);
   TCALLINGCONV(AAPCS);
Index: test/CodeGen/aarch64-vpcs.c
===
--- /dev/null
+++ test/CodeGen/aarch64-vpcs.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECKC
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -x c++ -o - %s | FileCheck %s -check-prefix=CHECKCXX
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -verify %s
+
+void __attribute__((aarch64_vector_pcs)) f(int *); // expected-warning {{calling convention 'aarch64_vector_pcs' ignored for this target}}
+
+// CHECKC: define void @g(
+// CHECKCXX: define void @_Z1gPi(
+void g(int *a) {
+
+// CHECKC: call aarch64_vector_pcs void @f(
+// CHECKCXX: call aarch64_vector_pcs void @_Z1fPi
+  f(a);
+}
+
+// CHECKC: declare aarch64_vector_pcs void @f(
+// CHECKCXX: declare aarch64_vector_pcs void @_Z1fPi
+
+void __attribute__((aarch64_vector_pcs)) h(int *a){ // expected-warning {{calling convention 'aarch64_vector_pcs' ignored for this target}}
+// CHECKC: define aarch64_vector_pcs void @h(
+// CHECKCXX: define aarch64_vector_pcs void @_Z1hPi(
+  f(a);
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -116,6 +116,7 @@
   case ParsedAttr::AT_Pascal:  \
   case ParsedAttr::AT_SwiftCall:   \
   case ParsedAttr::AT_VectorCall:  \
+  case ParsedAttr::AT_AArch64VectorPcs:\
   case ParsedAttr::AT_MSABI:   \
   case ParsedAttr::AT_SysVABI: \
   case ParsedAttr::AT_Pcs: \
@@ -6657,6 +6658,8 @@
 return createSimpleAttr(Ctx, Attr);
   case ParsedAttr::AT_VectorCall:
 return createSimpleAttr(Ctx, Attr);
+  case ParsedAttr::AT_AArch64VectorPcs:
+return createSimpleAttr(Ctx, Attr);
   case ParsedAttr::AT_Pcs: {
 // The attribute may have had a fixit applied where we treated an
 // identifier as a string literal.  The contents of the string are valid,
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4291,6 +4291,11 @@
AL.getAttributeSpellingListIndex()));
 return;
   }
+  case ParsedAttr::AT_AArch64VectorPcs:
+D->addAttr(::new(S.Context)
+   AArch64VectorPcsAttr(AL.getRange(), S.Context,
+AL.getAttributeSpellingListIndex()));
+return;
   case ParsedAttr::AT_IntelOclBicc:
 D->addAttr(::new (S.Context)
IntelOclBiccAttr(AL.getRange(), S.Context,
@@ -4368,6 +4373,9 @@
   case ParsedAttr::AT_VectorCall:
 CC = CC_X86VectorCall;
 break;
+  case ParsedAttr::AT_AArch64VectorPcs:
+CC = CC_AArch64VectorCall;
+break;
   case ParsedAttr::AT_RegCall:
 CC = CC_X86RegCall;
 break;
@@ -5840,14 +5848,14 @@
   if (AL.isDeclspecAttribute() || AL.isCXX11Attribute())
 checkAttributeAtMostNumArgs(S, AL, 1);
   else if (AL.isArgExpr(1) && AL.getArgAsExpr(1) &&
-   !S.checkStringLiteralArgumentAttr(AL, 1, Replacement))
-return;
-
-  if (!S.getLangOpts().CPlusPlus14 && AL.isCXX11Attribute() && !AL.isGNUScope())
-S.Diag(AL.getLoc(), diag::ext_cxx14_attr) << AL;
-
-  D->addAttr(::new (S.Context)
- DeprecatedAttr(AL.getRange(), S.Context, Str, Replacement,
+   !S.checkStringLiteralArgumentAttr(AL, 1, Replacement))
+return;
+
+  if (!S.getLangOpts().CPlusPlus14 && AL.isCXX11Attribute() && !AL.isGNUScope())
+S.Diag(AL.getLoc(), diag::ext_cxx14_attr) << AL;
+
+  D->addAttr(::new (S.Context)
+ DeprecatedAttr(AL

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Christoph Berg via Phabricator via cfe-commits]

ChristophBerg added a comment.

PostgreSQL 11 is now using LLVM to do JITing of SQL expressions. We'd need this 
feature to strip the build directory off the .bc bitcode files so the .deb 
packages build reproducibly.
@dankm: Are you still working on this? What can we do to help getting this move 
forward?


Repository:
  rC Clang

https://reviews.llvm.org/D49466



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

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+if (StringRef(A->getValue()) == "single")
+  return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
 SmallString<128> T(FinalOutput->getValue());

dblaikie wrote:
> If this function now takes the output file name - could it be simplified to 
> just always use that, rather than the conditional code that follows and tests 
> whether -o is specified and builds up something like the output file name & 
> uses the dwo suffix?
I am investigating this.



Comment at: test/CodeGen/split-debug-single-file.c:12
+//  RUN:   -enable-split-dwarf=split -split-dwarf-file %t.o -emit-obj -o %t.o 
%s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck 
--check-prefix=MODE-SPLIT %s
+//  MODE-SPLIT-NOT: .dwo

dblaikie wrote:
> This looks like an end-to-end test, which we don't usually do in Clang (or in 
> the LLVM project in general).
> 
> For example, the previous testing for split-dwarf had a driver test (that 
> tested only that the driver produced the right cc1 invocation) and a CodeGen 
> test (that tested that the right IR was produced), but I don't see any test 
> like this (that tested the resulting object file)?
> 
> I know there's a narrow gap in IR testing - things that don't go in the IR, 
> but instead go through MCOptions  & that the SplitDwarfFile is one of those.
> 
> So, yeah, in this case it's a bit of a test hole - if you're going to keep 
> this test, perhaps it could test assembly, rather than the object file? Since 
> it doesn't need complex parsing, etc, perhaps?
> So, yeah, in this case it's a bit of a test hole - if you're going to keep 
> this test, perhaps it could test assembly, rather than the object file? Since 
> it doesn't need complex parsing, etc, perhaps?

I am not sure assembly can help here. For example
`clang main.c -S -g -gsplit-dwarf` would create single asm file output anyways
and what I tried to check here is that we can either place .dwo sections into 
the
same output or into a different one depending on new option.

> For example, the previous testing for split-dwarf had a driver test (that 
> tested only that the driver produced the right cc1 invocation) and a CodeGen 
> test (that tested that the right IR was produced), but I don't see any test 
> like this (that tested the resulting object file)?

I think `split-debug-filename.c` do that?
See: 
https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/split-debug-filename.c#L18

Also, `relax.c`: 
https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/relax.c
And `mips-inline-asm-abi.c`:  
https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/mips-inline-asm-abi.c

Seems it is not common, but still acceptable?


https://reviews.llvm.org/D52296



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

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 173836.
gchatelet marked 6 inline comments as done.
gchatelet added a comment.

- Address comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488

Files:
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -config="{CheckOptions: [{key: "cppcoreguidelines-narrowing-conversions.WarnOnFloatingPointNarrowingConversion", value: 1}]}" -- -target x86_64-unknown-linux
 
 float ceil(float);
 namespace std {
@@ -9,47 +9,290 @@
 namespace floats {
 
 struct ConvertsToFloat {
-  operator float() const { return 0.5; }
+  operator float() const { return 0.5f; }
 };
 
-float operator "" _Pa(unsigned long long);
+float operator"" _float(unsigned long long);
 
-void not_ok(double d) {
+void narrow_double_to_int_not_ok(double d) {
   int i = 0;
   i = d;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = static_cast(d);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = ConvertsToFloat();
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-  i = 15_Pa;
+  i = 15_float;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
 }
 
-void not_ok_binary_ops(double d) {
+void narrow_double_to_int_not_ok_binary_ops(double d) {
   int i = 0;
   i += 0.5;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += d;
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   // We warn on the following even though it's not dangerous because there is no
   // reason to use a double literal here.
-  // TODO(courbet): Provide an automatic fix.
+  // TODO: Provide an automatic fix if the number is exactly representable in the destination type.
   i += 2.0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += 2.0f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
 
   i *= 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i /= 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += (double)0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+}
+
+double operator"" _double(unsigned long long);
+
+float narrow_double_to_float_retur

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:299
+  Rhs->isCXX11ConstantExpr(Context, &Constant)) {
+if (Constant.isFloat())
+  diagIfNarrowFloatingPointConstant(Context, SourceLoc, Lhs, Rhs, 
Constant);

JonasToth wrote:
> Why is the return here `true` even without diagnostic?
Good catch


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488



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

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

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

Szelethus planned changes to this revision.
Szelethus added a comment.

Here's how I'm planning to approach this issue:

- Split `MallocChecker` and `CStringChecker` up properly, introduce 
`MallocCheckerBase` and `CStringCheckerBase`. I'm a little unsure about how 
I'll do this, but I'll share my thoughts when I come up with something clever.
- Some checkers do not register themselves in all cases[1], which should 
probably be handled at a higher level, preferably in `Checkers.td`. Warnings 
could be emitted when a checker is explicitly enabled but will not be 
registered.
- Once all checkers in all cases properly register themselves, refactor this 
patch to introduce dependencies in between the subcheckers of `MallocChecker` 
and `CStringChecker`. This will imply that every registry function registers 
one, and only one checker.
- Move `CheckerManager::registerChecker` out of the registry functions, 
rename them from `register##CHECKERNAME` to `setup##CHECKERNAME`, and supply 
the //mutable// checker object and the //immutable// `CheckerManager`. The 
downside is that all checkers will have to be default constructible, and their 
fields will have to be set up in `setup##CHECKERNAME`, but I see this as an 
acceptable tradeoff for the extra security.
  - This will make it impossible to affect other checkers. I'm a little unsure 
whether this is achievable considering how intertwined everything is in 
`MallocChecker`, but even if the supplied `CheckerManager` will have to 
non-const, `getCurrentCheckerName` and `setCurrentCheckerName` could be 
eliminated for sure.
- Properly document why `CheckerRegistry` and `CheckerManager` are separate 
entities, how are the tblgen files processed, how are dependencies handled, and 
so on.
- Rebase my local checker option-related branches, and celebrate. Unless 
another skeleton falls out of the closet. You never know I guess.

[1]

  void ento::registerObjCDeallocChecker(CheckerManager &Mgr) {
const LangOptions &LangOpts = Mgr.getLangOpts();
// These checker only makes sense under MRR.
if (LangOpts.getGC() == LangOptions::GCOnly || LangOpts.ObjCAutoRefCount)
  return;
  
Mgr.registerChecker();
  }


Repository:
  rC Clang

https://reviews.llvm.org/D54438



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

[PATCH] D54269: Introduce shard storage to auto-index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/index/Background.cpp:52
+llvm::SmallString<128>
+getShardPathFromFilePath(llvm::SmallString<128> ShardRoot,
+ llvm::StringRef FilePath) {

nit: these micro-optimizations with SmallString seem unlikely to be worthwhile 
- maybe just take StringRef and return string?



Comment at: clangd/index/Background.cpp:109
+
+SmallString<128> getAbsolutePath(const tooling::CompileCommand &Cmd) {
+  SmallString<128> AbsolutePath;

absoluteFilePath? (otherwise it's not clear what the path is to)



Comment at: clangd/index/Background.cpp:237
+  if (IndexStorage)
+loadShards(IndexStorage.get(), Cmds);
+  else

this absolutely can't be done synchronously (it's much slower than calling 
getAllCompileCommands - this could block clangd for tens of seconds when 
opening the first file).

Can we drop shard-loading in BackgroundIndex from this patch?



Comment at: clangd/index/Background.cpp:256
   Queue.push_back(Bind(
-  [this](tooling::CompileCommand Cmd) {
+  [this](tooling::CompileCommand Cmd,
+ std::shared_ptr IndexStorage) {

you can capture indexstorage by value



Comment at: clangd/index/Background.h:34
+private:
+  static 
std::function(llvm::StringRef)>
+  Factory;

I don't think we should be using a static mutable factory here.

I think I worded the previous comment about exposing `DiskBackedIndexStorage` 
badly, what I meant was something like: `static 
unique_ptr 
BackgroundIndexStorage::createDiskStorage(StringRef dir)`

This would be separate from how the storage strategy is configured on the 
background index.




Comment at: clangd/index/Background.h:34
+private:
+  static 
std::function(llvm::StringRef)>
+  Factory;

sammccall wrote:
> I don't think we should be using a static mutable factory here.
> 
> I think I worded the previous comment about exposing `DiskBackedIndexStorage` 
> badly, what I meant was something like: `static 
> unique_ptr 
> BackgroundIndexStorage::createDiskStorage(StringRef dir)`
> 
> This would be separate from how the storage strategy is configured on the 
> background index.
> 
do we need `shared_ptr` here?
ISTM either the BackgroundIndex is going to be responsible for caching (in 
which case it should receive unique_ptr) or the factory is responsible for 
caching (in which case it can own the functions, and the BackgroundIndex can 
get a non-owning raw pointer)



Comment at: clangd/index/Background.h:38
+public:
+  using FileDigest = decltype(llvm::SHA1::hash({}));
+

unused?



Comment at: clangd/index/Background.h:42
+  // retrieved later on with the same identifier.
+  virtual bool storeShard(llvm::StringRef ShardIdentifier,
+  IndexFileOut Shard) const = 0;

Prefer llvm::Error over bool, or no return value if you don't think we ever 
want to handle it.



Comment at: clangd/index/Background.h:45
+
+  // Retrieves the shard if found, also returns hash for the source file that
+  // generated this shard.

doc stale



Comment at: clangd/index/Background.h:48
+  virtual llvm::Expected
+  retrieveShard(llvm::StringRef ShardIdentifier) const = 0;
+

nit: I think "loadShard" would better emphasize the relationship with 
`storeShard`, as load/store is such a common pair. (read/write would similarly 
work).
Nothing wrong with "retrieve" per se, I just think the pairing is better.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[Erich Keane via Phabricator via cfe-commits]

erichkeane added subscribers: aaron.ballman, erichkeane.
erichkeane added a comment.

You'll not be able to get the C++ spelling without it getting into the 
standard.  Additionally, it seems easy enough to calculate upon need, so 
storing it in the AST is a waste.  @aaron.ballman is likely another person who 
can review this.




Comment at: docs/LanguageExtensions.rst:1093
   library.
+* ``__is_trivially_relocatable`` (Clang): Determines whether moving an object
+  of type ``type``, and then destroying the source object, is functionally

How would this behave with unions?  I don't see any exclusions happening on 
union-ness.  A CXXRecordDecl can be a union as well as a class.



Comment at: include/clang/AST/DeclCXX.h:482
+/// and a defaulted destructor.
+unsigned IsNaturallyTriviallyRelocatable : 1;
+

Typically we'd have this calculated when requested rather than stored. I 
suspect using a bit for something like this isn't going to be terribly 
acceptable.



Comment at: include/clang/Basic/Attr.td:2096
+def TriviallyRelocatable : InheritableAttr {
+  let Spellings = [CXX11<"", "trivially_relocatable", 200809>,
+   CXX11<"clang", "trivially_relocatable">];

This spelling is almost definitely not acceptable until it is in the standard.  
Also, why specify that it was added in 2008?



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8207
 
+def err_attribute_missing_on_first_decl : Error<
+  "type %0 declared %1 after its first declaration">;

I'm shocked that there isn't a different diagnostic to do this same thing.  
@aaron.ballman likely knows better...  I haven't seen the usage yet, but I 
presume you don't necessarily want a behavior that doesn't allow forward 
declarations.



Comment at: lib/AST/Type.cpp:2234
+bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
+  QualType T = Context.getBaseElementType(*this);
+  if (T->isIncompleteType())

You likely want to canonicalize here.



Comment at: lib/Parse/ParseDeclCXX.cpp:3811
   case ParsedAttr::AT_CXX11NoReturn:
+  case ParsedAttr::AT_TriviallyRelocatable:
 return true;

A note for future reviewers, after the C++11 spelling is removed, this likely 
needs to go as well.


Repository:
  rC Clang

https://reviews.llvm.org/D50119



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

Re: [PATCH] D54405: Record whether a AST Matcher constructs a Node

[Zachary Turner via cfe-commits]

I don’t really have much more to add here except to refer you to the style
guide:

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Specifically this line: “Use auto if and only if it makes the code more
readable or easier to maintain.”

Given that 2 out of 2 reviewers who have looked at this have said it did
not make the code more readable or easier to maintain for them , and have
further said that they feel the return type is not “obvious from the
context”, i do not believe this usage fits within our style guidelines.

I don’t think it’s worth beating on this anymore though, because this is a
lot of back and forth over something that doesn’t actually merit this much
discussion. So in the interest of conforming to the style guide above,
please remove auto and then start a thread on llvm-dev if you disagree with
the policy
On Tue, Nov 13, 2018 at 1:57 AM Stephen Kelly via Phabricator <
revi...@reviews.llvm.org> wrote:

> steveire added inline comments.
>
>
> 
> Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:77
> +  internal::MatcherDescriptor *matchDescriptor, StringRef
> MatcherName) {
> +auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<
> +typename
> ast_matchers::internal::VariadicAllOfMatcher::Type>();
> 
> zturner wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > zturner wrote:
> > > > > steveire wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > steveire wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Mildly not keen on the use of `auto` here. It's a factory
> function, so it kind of names the resulting type, but it also looks like
> the type will be
> `ast_matchers::internal::VariadicAllOfMatcher::Type` from the
> template argument, which is incorrect.
> > > > > > > > There is no reason to assume that taking a template argument
> means that type is result.
> > > > > > > >
> > > > > > > > The method is `getFrom` which decreases the ambiguity still
> further.
> > > > > > > >
> > > > > > > > Spelling out the type doesn't add anything useful. This
> should be ok.
> > > > > > > > There is no reason to assume that taking a template argument
> means that type is result.
> > > > > > >
> > > > > > > Aside from all the other places that do exactly that (getAs<>,
> cast<>, dyn_cast<>, castAs<>, and so on). Generally, when we have a
> function named get that takes a template type argument, the result when
> seen in proximity to auto is the template type.
> > > > > > >
> > > > > > > > Spelling out the type doesn't add anything useful. This
> should be ok.
> > > > > > >
> > > > > > > I slept on this one and fall on the other side of it; using
> auto hides information that tripped up at least one person when reading the
> code, so don't use auto. It's not clear enough what the resulting type will
> be.
> > > > > > I put this in the category of requiring
> > > > > >
> > > > > > SomeType ST = SomeType::create();
> > > > > >
> > > > > > instead of
> > > > > >
> > > > > > auto ST = SomeType::create();
> > > > > >
> > > > > > `ast_type_traits::ASTNodeKind` is already on that line and you
> want to see it again.
> > > > > >
> > > > > FWIW I'm with Aaron here.  Im' not familiar at all with this
> codebase, and looking at the code, my first instinct is "the result type is
> probably `ast_matchers::internal::VariadicAllOfMatcher::Type`".
> According to Aaron's earlier comment, that is incorrect, so there's at
> least 1 data point that it hinders readability.
> > > > >
> > > > > So, honest question.  What *is* the return type here?
> > > > > So, honest question. What *is* the return type here?
> > > >
> > > > Much to my surprise, it's `ASTNodeKind`. It was entirely unobvious
> to me that this was a factory function.
> > > @zturner Quoting myself:
> > >
> > > > `ast_type_traits::ASTNodeKind` is already on that line and you want
> to see it again.
> > >
> > > The expression is `getFromNodeKind`. There is a pattern of
> `SomeType::fromFooKind()` returning a `SomeType`. This is not so
> unusual.
> > >
> > >
> > Note that at the top of this file there's already a `using namespace
> clang::ast_type_traits;`  So if you're worried about verbosity, then you
> can remove the `ast_type_traits::`, remove the `auto`, and the net effect
> is that the overall line will end up being shorter.
> The funny thing is - if you see a line like
>
> Parser CodeParser = Parser::getFromArgs(Args);
>
> you have no idea what type `Parser` is!
>
> To start, it could be `clang::Parser` or
> `clang::ast_matchers::dynamic::Parser`, depending on a `using namespace`
> which might appear any distance up in the file. It is not uncommon for
> clang files to be thousands of lines lone.
>
> Or it could be inside a template with `template`, or
> there could be a `using Parser = Foo;` any distance up in the file.
>
> Parser CodeParser = Parser::getFromArgs(Args);
>
> is no more helpful than
>
> auto CodeParser = Pars

[PATCH] D54405: Record whether a AST Matcher constructs a Node

[Mailing List "cfe-commits" via Phabricator via cfe-commits]

cfe-commits added a subscriber: aaron.ballman.
cfe-commits added a comment.

I don’t really have much more to add here except to refer you to the style
guide:

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Specifically this line: “Use auto if and only if it makes the code more
readable or easier to maintain.”

Given that 2 out of 2 reviewers who have looked at this have said it did
not make the code more readable or easier to maintain for them , and have
further said that they feel the return type is not “obvious from the
context”, i do not believe this usage fits within our style guidelines.

I don’t think it’s worth beating on this anymore though, because this is a
lot of back and forth over something that doesn’t actually merit this much
discussion. So in the interest of conforming to the style guide above,
please remove auto and then start a thread on llvm-dev if you disagree with
the policy

- F7542440: msg-532-122.txt 


Repository:
  rC Clang

https://reviews.llvm.org/D54405



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

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 173842.
hokein marked an inline comment as done.
hokein added a comment.

update the patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53427

Files:
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/Merge.cpp
  clangd/index/Serialization.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/YAMLSerialization.cpp
  clangd/index/dex/Dex.cpp
  unittests/clangd/BackgroundIndexTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/DexTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/SerializationTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -53,8 +53,10 @@
   return (arg.Name + arg.CompletionSnippetSuffix).str() == S;
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
-MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; }
+MATCHER_P(DeclURI, P, "") {
+  return StringRef(arg.CanonicalDeclaration.FileURI) == P;
+}
+MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
 MATCHER_P(IncludeHeader, P, "") {
   return (arg.IncludeHeaders.size() == 1) &&
  (arg.IncludeHeaders.begin()->IncludeHeader == P);
Index: unittests/clangd/SerializationTests.cpp
===
--- unittests/clangd/SerializationTests.cpp
+++ unittests/clangd/SerializationTests.cpp
@@ -111,7 +111,7 @@
   EXPECT_EQ(Sym1.Signature, "");
   EXPECT_EQ(Sym1.Documentation, "Foo doc");
   EXPECT_EQ(Sym1.ReturnType, "int");
-  EXPECT_EQ(Sym1.CanonicalDeclaration.FileURI, "file:///path/foo.h");
+  EXPECT_EQ(StringRef(Sym1.CanonicalDeclaration.FileURI), "file:///path/foo.h");
   EXPECT_EQ(Sym1.Origin, SymbolOrigin::Static);
   EXPECT_TRUE(Sym1.Flags & Symbol::IndexedForCodeCompletion);
   EXPECT_FALSE(Sym1.Flags & Symbol::Deprecated);
@@ -122,7 +122,8 @@
   EXPECT_THAT(Sym2, QName("clang::Foo2"));
   EXPECT_EQ(Sym2.Signature, "-sig");
   EXPECT_EQ(Sym2.ReturnType, "");
-  EXPECT_EQ(Sym2.CanonicalDeclaration.FileURI, "file:///path/bar.h");
+  EXPECT_EQ(llvm::StringRef(Sym2.CanonicalDeclaration.FileURI),
+"file:///path/bar.h");
   EXPECT_FALSE(Sym2.Flags & Symbol::IndexedForCodeCompletion);
   EXPECT_TRUE(Sym2.Flags & Symbol::Deprecated);
 
@@ -134,7 +135,7 @@
testing::SizeIs(1;
   auto Ref1 = ParsedYAML->Refs->begin()->second.front();
   EXPECT_EQ(Ref1.Kind, RefKind::Reference);
-  EXPECT_EQ(Ref1.Location.FileURI, "file:///path/foo.cc");
+  EXPECT_EQ(StringRef(Ref1.Location.FileURI), "file:///path/foo.cc");
 }
 
 std::vector YAMLFromSymbols(const SymbolSlab &Slab) {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -36,7 +36,7 @@
  std::make_tuple(Range.start.line, Range.start.character,
  Range.end.line, Range.end.character);
 }
-MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
+MATCHER_P(FileURI, F, "") { return StringRef(arg.Location.FileURI) == F; }
 
 TEST(SymbolLocation, Position) {
   using Position = SymbolLocation::Position;
@@ -222,7 +222,7 @@
 
   Symbol M = mergeSymbol(L, R);
   EXPECT_EQ(M.Name, "Foo");
-  EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:///left.h");
+  EXPECT_EQ(StringRef(M.CanonicalDeclaration.FileURI), "file:///left.h");
   EXPECT_EQ(M.References, 3u);
   EXPECT_EQ(M.Signature, "()");
   EXPECT_EQ(M.CompletionSnippetSuffix, "{$1:0}");
@@ -241,14 +241,14 @@
   R.Name = "right";
 
   Symbol M = mergeSymbol(L, R);
-  EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/left.h");
-  EXPECT_EQ(M.Definition.FileURI, "");
+  EXPECT_EQ(StringRef(M.CanonicalDeclaration.FileURI), "file:/left.h");
+  EXPECT_EQ(StringRef(M.Definition.FileURI), "");
   EXPECT_EQ(M.Name, "left");
 
   R.Definition.FileURI = "file:/right.cpp"; // Now right will be favored.
   M = mergeSymbol(L, R);
-  EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/right.h");
-  EXPECT_EQ(M.Definition.FileURI, "file:/right.cpp");
+  EXPECT_EQ(StringRef(M.CanonicalDeclaration.FileURI), "file:/right.h");
+  EXPECT_EQ(StringRef(M.Definition.FileURI), "file:/right.cpp");
   EXPECT_EQ(M.Name, "right");
 }
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -31,19 +31,21 @@
 using testing::IsEmpty;
 using testing::Pair;
 using testing::UnorderedElementsAre;
+using namespace llvm;
 
 MATCHER_P(RefRange, Range, "") {
   return std::make_tuple(arg.Location.Start.line(), arg.Location.Start.co

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/index/Index.h:84
 inline bool operator==(const SymbolLocation &L, const SymbolLocation &R) {
-  return std::tie(L.FileURI, L.Start, L.End) ==
- std::tie(R.FileURI, R.Start, R.End);
+  return std::make_tuple(llvm::StringRef(L.FileURI), L.Start, L.End) ==
+ std::make_tuple(llvm::StringRef(R.FileURI), R.Start, R.End);

MaskRay wrote:
> The `StringRef(const char*)` constructor calls `strlen`. Do you believe this 
> does not have noticeable performance issue?
> 
> (A minor point: `tuple::operator<` calls `operator<` of its components in a 
> not very efficient way. The compiler does not know `compare(a, b) < 0` 
> implies `compare(b, a) > 0` and `strcmp` may end up being called twice.)
Thanks for the suggestions. The code here has an addition `strlen` cost. I 
updated the patch to avoid this additional cost and the potential double `<` 
calls of string.  


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53427



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

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 173844.
gchatelet added a comment.

- Remove debugging leftover


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488

Files:
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -config="{CheckOptions: [{key: "cppcoreguidelines-narrowing-conversions.WarnOnFloatingPointNarrowingConversion", value: 1}]}" -- -target x86_64-unknown-linux
 
 float ceil(float);
 namespace std {
@@ -9,47 +9,290 @@
 namespace floats {
 
 struct ConvertsToFloat {
-  operator float() const { return 0.5; }
+  operator float() const { return 0.5f; }
 };
 
-float operator "" _Pa(unsigned long long);
+float operator"" _float(unsigned long long);
 
-void not_ok(double d) {
+void narrow_double_to_int_not_ok(double d) {
   int i = 0;
   i = d;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = static_cast(d);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = ConvertsToFloat();
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-  i = 15_Pa;
+  i = 15_float;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
 }
 
-void not_ok_binary_ops(double d) {
+void narrow_double_to_int_not_ok_binary_ops(double d) {
   int i = 0;
   i += 0.5;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += d;
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   // We warn on the following even though it's not dangerous because there is no
   // reason to use a double literal here.
-  // TODO(courbet): Provide an automatic fix.
+  // TODO: Provide an automatic fix if the number is exactly representable in the destination type.
   i += 2.0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += 2.0f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
 
   i *= 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i /= 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += (double)0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+}
+
+double operator"" _double(unsigned long long);
+
+float narrow_double_to_float_return() {
+  return 0.5;
+  // CHECK-ME

[PATCH] D54466: [Analyzer] [WIP] Iterator Checkers - Use the base region of C++ Base Object Regions (recursively) for iterators stored in a region

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

Szelethus added a comment.

Hmmm, shouldn't we add this to `MemRegion`'s interface instead?


Repository:
  rC Clang

https://reviews.llvm.org/D54466



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

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Jonas Toth via Phabricator via cfe-commits]

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

So, finally LGTM :)
Please give @aaron.ballman a chance to comment, as he reviewed too.

Thanks for your patch!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488



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

[PATCH] D54473: [sanitizers] Initial implementation for -fsanitize=init-locals

[Alexander Potapenko via Phabricator via cfe-commits]

glider created this revision.
glider added reviewers: kcc, rjmccall, rsmith.
Herald added a subscriber: cfe-commits.

This patch adds a new feature, -fsanitize=init-locals, which generates zero 
initializers for uninitialized local variables.

There's been discussions in the security community about the impact of 
zero-initializing all locals to prevent information leaks. The new feature 
shall help evaluating the pros and cons of such an approach.

Credits for the code go to Daniel Micay (original patch is at 
https://github.com/AndroidHardeningArchive/platform_external_clang/commit/776a0955ef6686d23a82d2e6a3cbd4a6a882c31c)


Repository:
  rC Clang

https://reviews.llvm.org/D54473

Files:
  include/clang/Basic/Sanitizers.def
  lib/CodeGen/CGDecl.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/sanitize-init-locals.c

Index: test/CodeGen/sanitize-init-locals.c
===
--- test/CodeGen/sanitize-init-locals.c
+++ test/CodeGen/sanitize-init-locals.c
@@ -0,0 +1,42 @@
+// Test for -fsanitize=init-locals.
+
+// RUN: %clang_cc1 %s -O0 -triple x86_64-unknown-linux-gnu -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -O0 -triple x86_64-unknown-linux-gnu -fsanitize=init-locals -emit-llvm -o - | FileCheck -check-prefixes=CHECK,CHECK-INIT-LOCALS %s
+// RUN: %clang_cc1 %s -O0 -triple x86_64-unknown-linux-gnu -Wuninitialized -emit-llvm -o - 2>&1 | FileCheck -check-prefix=CHECK-WUNINITIALIZED %s
+// RUN: %clang_cc1 %s -O0 -triple x86_64-unknown-linux-gnu -Wuninitialized -fsanitize=init-locals -emit-llvm -o - 2>&1 | FileCheck -check-prefix=CHECK-WUNINITIALIZED %s
+
+
+// CHECK: @testSanitizeInitLocals.local_array_init_part = private unnamed_addr constant [5 x i32] [i32 0, i32 1, i32 2, i32 0, i32 0], align 16
+// CHECK: @testSanitizeInitLocals.local_array_init = private unnamed_addr constant [5 x i32] [i32 0, i32 1, i32 2, i32 3, i32 4], align 16
+
+// CHECK: %local_int_uninit = alloca i32, align 4
+// CHECK: %local_int_init_0 = alloca i32, align 4
+// CHECK: %local_int_init = alloca i32, align 4
+// CHECK: %local_array_uninit = alloca [5 x i32], align 16
+// CHECK: %local_array_init_part = alloca [5 x i32], align 16
+// CHECK: %local_array_init = alloca [5 x i32], align 16
+
+// CHECK-INIT-LOCALS: store i32 0, i32* %local_int_uninit, align 4
+// CHECK: store i32 0, i32* %local_int_init_0, align 4
+// CHECK: store i32 1, i32* %local_int_init, align 4
+//
+// CHECK-INIT-LOCALS: [[CAST0:%.*]] = {{.*}}%local_array_uninit
+// CHECK-INIT-LOCALS: call void @llvm.memset{{.*}}({{.*}}[[CAST0]], i8 0, i64 20
+// CHECK: [[CAST1:%.*]] = {{.*}}%local_array_init_part
+// CHECK: call void @llvm.memcpy{{.*}}({{.*}}[[CAST1]], {{.*}}@testSanitizeInitLocals.local_array_init_part{{.*}}, {{.*}} 20
+// CHECK: [[CAST2:%.*]] = {{.*}}%local_array_init
+// CHECK: call void @llvm.memcpy{{.*}}({{.*}}[[CAST2]], {{.*}}@testSanitizeInitLocals.local_array_init{{.*}}, {{.*}} 20
+
+// CHECK-WUNINITIALIZED: warning: variable 'local_int_uninit' is uninitialized when used here
+
+int testSanitizeInitLocals(void)
+{
+  const int local_int_uninit;
+  const int local_int_init_0 = 0;
+  const int local_int_init = 1;
+  const int local_array_uninit[5];
+  const int local_array_init_part[5] = {0, 1, 2};
+  const int local_array_init[5] = {0, 1, 2, 3, 4};
+  return local_int_uninit;
+}
+
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -819,7 +819,7 @@
 
   SanitizerMask Res = (Undefined & ~Vptr & ~Function) | (CFI & ~CFIICall) |
   CFICastStrict | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds;
+  ImplicitConversion | Nullability | LocalBounds | InitLocals;
   if (getTriple().getArch() == llvm::Triple::x86 ||
   getTriple().getArch() == llvm::Triple::x86_64 ||
   getTriple().getArch() == llvm::Triple::arm ||
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1434,7 +1434,8 @@
 return;
   }
 
-  if (isTrivialInitializer(Init))
+  bool trivial = isTrivialInitializer(Init);
+  if (trivial && !getLangOpts().Sanitize.has(SanitizerKind::InitLocals))
 return;
 
   // Check whether this is a byref variable that's potentially
@@ -1445,8 +1446,15 @@
   Address Loc =
 capturedByInit ? emission.Addr : emission.getObjectAddress(*this);
 
+  bool constantAggregate = emission.IsConstantAggregate;
+
   llvm::Constant *constant = nullptr;
-  if (emission.IsConstantAggregate || D.isConstexpr()) {
+  if (trivial) {
+QualType Ty = D.getType();
+constant = CGM.EmitNullConstant(Ty);
+if (Ty->isArrayType() || Ty->isRecordType())
+  constantAggregate = true;
+  } else if (emission.IsConstantAggregate || D.isConstexpr()) {
 assert(!capturedByInit && "constant init contains a capturing blo

[PATCH] D54429: [analyzer] Creating standard Sphinx documentation

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp updated this revision to Diff 173846.
dkrupp added a comment.

-scanbuild and xcode pictures are included now
-intro text ("What is Static Analysis?" etc.) are put under the Introduction 
section
-Download section is created, but I am not sure how well was the this Mac OSX 
binary release section was maintained. Should users really download from this 
site or through a package manager instead?

I will transform all the subpages after we come to an agreement that this is 
the way to go. For now, there are links to the old sub-pages from the main page.

@dcoughlin what do you think?

This is the current status.
Done:
Main: https://clang-analyzer.llvm.org/
Main/User manual/available checks: 
https://clang-analyzer.llvm.org/available_checks.html
Main/Mailing Lists

Not done:
Main/Filing Bugs: https://clang-analyzer.llvm.org/filing_bugs.html
Main/User manual/Obtaining the analyzer: 
https://clang-analyzer.llvm.org/installation.html
Main/User manual/Command line usage: 
https://clang-analyzer.llvm.org/scan-build.html
Main/User manual/Using within XCode: https://clang-analyzer.llvm.org/xcode.html
Main/Development/Checker Developer Manual: 
https://clang-analyzer.llvm.org/checker_dev_manual.html
Main/Development/Open Projects: 
https://clang-analyzer.llvm.org/open_projects.html
Main/Development/Potential Future Checkers: 
https://clang-analyzer.llvm.org/potential_checkers.html
Main/User manual/Source level annotations: 
https://clang-analyzer.llvm.org/annotations.html
Main/User manual/FAQ: https://clang-analyzer.llvm.org/faq.html


https://reviews.llvm.org/D54429

Files:
  docs/ClangStaticAnalyzer.rst
  docs/analyzer/DesignDiscussions/IPA.rst
  docs/analyzer/DesignDiscussions/InitializerLists.rst
  docs/analyzer/DesignDiscussions/RegionStore.rst
  docs/analyzer/DesignDiscussions/nullability.rst
  docs/analyzer/IPA.txt
  docs/analyzer/RegionStore.txt
  docs/analyzer/checkers.rst
  docs/analyzer/checkers/UninitializedObject.rst
  docs/analyzer/images/analyzer_xcode_html.png
  docs/analyzer/nullability.rst
  docs/conf.py
  docs/index.rst

Index: docs/index.rst
===
--- docs/index.rst
+++ docs/index.rst
@@ -23,6 +23,7 @@
AttributeReference
DiagnosticsReference
CrossCompilation
+   ClangStaticAnalyzer
ThreadSafetyAnalysis
AddressSanitizer
ThreadSanitizer
Index: docs/conf.py
===
--- docs/conf.py
+++ docs/conf.py
@@ -65,7 +65,7 @@
 
 # List of patterns, relative to source directory, that match files and
 # directories to ignore when looking for source files.
-exclude_patterns = ['_build', 'analyzer']
+exclude_patterns = ['_build']
 
 # The reST default role (used for this markup: `text`) to use for all documents.
 #default_role = None
Index: docs/analyzer/nullability.rst
===
--- docs/analyzer/nullability.rst
+++ /dev/null
@@ -1,92 +0,0 @@
-
-Nullability Checks
-
-
-This document is a high level description of the nullablility checks.
-These checks intended to use the annotations that is described in this
-RFC: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2015-March/041798.html.
-
-Let's consider the following 2 categories:
-
-1) nullable
-
-
-If a pointer 'p' has a nullable annotation and no explicit null check or assert, we should warn in the following cases:
-- 'p' gets implicitly converted into nonnull pointer, for example, we are passing it to a function that takes a nonnull parameter.
-- 'p' gets dereferenced
-
-Taking a branch on nullable pointers are the same like taking branch on null unspecified pointers.
-
-Explicit cast from nullable to nonnul::
-
-__nullable id foo;
-id bar = foo;
-takesNonNull((_nonnull) bar); <— should not warn here (backward compatibility hack)
-anotherTakesNonNull(bar); <— would be great to warn here, but not necessary(*)
-
-Because bar corresponds to the same symbol all the time it is not easy to implement the checker that way the cast only suppress the first call but not the second. For this reason in the first implementation after a contradictory cast happens, I will treat bar as nullable unspecified, this way all of the warnings will be suppressed. Treating the symbol as nullable unspecified also has an advantage that in case the takesNonNull function body is being inlined, the will be no warning, when the symbol is dereferenced. In case I have time after the initial version I might spend additional time to try to find a more sophisticated solution, in which we would produce the second warning (*).
- 
-2) nonnull
-
-
-- Dereferencing a nonnull, or sending message to it is ok.
-- Converting nonnull to nullable is Ok.
-- When there is an explicit cast from nonnull to nullable I will trust the cast (it is probable there for a reason, because this cast does not suppress any warnings or errors).
-- But what sho

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

In https://reviews.llvm.org/D53488#1296940, @JonasToth wrote:

> So, finally LGTM :)
>  Please give @aaron.ballman a chance to comment, as he reviewed too.
>
> Thanks for your patch!


Thank you for bearing with me.
Waiting for input from @aaron.ballman


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488



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

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/index/Index.h:85
+  assert(L.FileURI && R.FileURI);
+  int Cmp = std::strcmp(L.FileURI, R.FileURI);
+  return Cmp == 0 && std::tie(L.Start, L.End) == std::tie(R.Start, R.End);

nit: I think it's more idiomatic just to inline as
```return !strcmp(L.FileURI, R.FileURI) && ...```



Comment at: clangd/index/Index.h:93
+return Cmp < 0;
+  return std::tie(L.Start, L.End) < std::tie(R.Start, R.End);
 }

tie(strcmp(L.FileURI, R.FileURI), ...) < tie(0, ...)?



Comment at: clangd/index/Index.h:306
+llvm::StringRef S(P);
+CB(S);
+P = S.data();

```assert (!S.data()[S.size()] && "Visited StringRef must be null-terminated")



Comment at: clangd/index/Merge.cpp:118
   bool MergeIncludes =
-  L.Definition.FileURI.empty() == R.Definition.FileURI.empty();
+  !std::strlen(L.Definition.FileURI) == !std::strlen(R.Definition.FileURI);
   Symbol S = PreferR ? R : L;// The target symbol we're merging into.

Since this reads awkwardly anyway...
```bool(*L.Definition.FileURI) == bool(*R.definition.FileURI)```
is shorter and faster


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53427



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

[PATCH] D54473: [sanitizers] Initial implementation for -fsanitize=init-locals

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/clang/Basic/Sanitizers.def:163
+// Initialize local variables.
+SANITIZER("init-locals", InitLocals)
+

Unless i'm mistaken, I suspect you may see some surprising behavior here, 
unfortunately.
[[ https://bugs.llvm.org/show_bug.cgi?id=39425 | Bug 39425 - SanitizerOrdinal 
is out of bits ]]


Repository:
  rC Clang

https://reviews.llvm.org/D54473



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

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+if (StringRef(A->getValue()) == "single")
+  return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
 SmallString<128> T(FinalOutput->getValue());

grimar wrote:
> dblaikie wrote:
> > If this function now takes the output file name - could it be simplified to 
> > just always use that, rather than the conditional code that follows and 
> > tests whether -o is specified and builds up something like the output file 
> > name & uses the dwo suffix?
> I am investigating this.
So what I see now is that when the function becomes:

```
const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
  const InputInfo &Output) {
  SmallString<128> T(Output.getFilename());
  llvm::sys::path::replace_extension(T, "dwo");
  return Args.MakeArgString(T);
}
```

Then no clang tests (check-clang) fail. This does not seem normal to me.
I would expect that such a change should break some `-fdebug-compilation-dir` 
relative 
logic at least and I suspect we just have no test(s) atm.

What about if I add test case(s) and post a follow-up refactor patch for this 
place?
(I started work on it, but it will take some time).


https://reviews.llvm.org/D52296



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

[PATCH] D54475: [clangd] Allow observation of changes to global CDBs.

[Sam McCall via Phabricator via cfe-commits]

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

Currently, changes *within* CDBs are not tracked (CDB has no facility to do so).
However, discovery of new CDBs are tracked (all files are marked as modified).
Also, files whose compilation commands are explicitly set are marked modified.

The intent is to use this for auto-index. Newly discovered files will be indexed
with low priority.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54475

Files:
  clangd/Function.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  unittests/clangd/GlobalCompilationDatabaseTests.cpp

Index: unittests/clangd/GlobalCompilationDatabaseTests.cpp
===
--- unittests/clangd/GlobalCompilationDatabaseTests.cpp
+++ unittests/clangd/GlobalCompilationDatabaseTests.cpp
@@ -88,6 +88,16 @@
   ElementsAre("clang", testPath("foo.cc"), "-DA=6"));
 }
 
+TEST_F(OverlayCDBTest, Watch) {
+  OverlayCDB Inner(nullptr);
+  OverlayCDB Outer(&Inner);
+
+  std::vector> Changes;
+  Outer.watch([&](const std::vector &ChangedFiles) {
+Changes.push_back(ChangedFiles);
+  });
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -11,6 +11,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_GLOBALCOMPILATIONDATABASE_H
 
 #include "Path.h"
+#include "Function.h"
 #include "llvm/ADT/StringMap.h"
 #include 
 #include 
@@ -41,8 +42,15 @@
   /// Clangd should treat the results as unreliable.
   virtual tooling::CompileCommand getFallbackCommand(PathRef File) const;
 
-  /// FIXME(ibiryukov): add facilities to track changes to compilation flags of
-  /// existing targets.
+  using CommandChanged = Event>;
+  /// The callback is notified when files may have new compile commands.
+  /// The argument is a list of full file paths.
+  CommandChanged::Subscription watch(CommandChanged::Listener L) {
+return OnCommandChanged.observe(std::move(L));
+  }
+
+protected:
+  mutable CommandChanged OnCommandChanged;
 };
 
 /// Gets compile args from tooling::CompilationDatabases built for parent
@@ -61,7 +69,8 @@
 
 private:
   tooling::CompilationDatabase *getCDBForFile(PathRef File) const;
-  tooling::CompilationDatabase *getCDBInDirLocked(PathRef File) const;
+  std::pair
+  getCDBInDirLocked(PathRef File) const;
 
   mutable std::mutex Mutex;
   /// Caches compilation databases loaded from directories(keys are
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -49,17 +49,17 @@
   return None;
 }
 
-tooling::CompilationDatabase *
+std::pair
 DirectoryBasedGlobalCompilationDatabase::getCDBInDirLocked(PathRef Dir) const {
   // FIXME(ibiryukov): Invalidate cached compilation databases on changes
   auto CachedIt = CompilationDatabases.find(Dir);
   if (CachedIt != CompilationDatabases.end())
-return CachedIt->second.get();
+return {CachedIt->second.get(), true};
   std::string Error = "";
   auto CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error);
   auto Result = CDB.get();
   CompilationDatabases.insert(std::make_pair(Dir, std::move(CDB)));
-  return Result;
+  return {Result, false};
 }
 
 tooling::CompilationDatabase *
@@ -69,14 +69,20 @@
   path::is_absolute(File, path::Style::windows)) &&
  "path must be absolute");
 
+  tooling::CompilationDatabase * CDB = nullptr;
+  bool Cached;
   std::lock_guard Lock(Mutex);
-  if (CompileCommandsDir)
-return getCDBInDirLocked(*CompileCommandsDir);
-  for (auto Path = path::parent_path(File); !Path.empty();
-   Path = path::parent_path(Path))
-if (auto CDB = getCDBInDirLocked(Path))
-  return CDB;
-  return nullptr;
+  if (CompileCommandsDir) {
+std::tie(CDB, Cached) = getCDBInDirLocked(*CompileCommandsDir);
+  } else {
+for (auto Path = path::parent_path(File); !CDB && !Path.empty();
+ Path = path::parent_path(Path)) {
+  std::tie(CDB, Cached) = getCDBInDirLocked(Path);
+}
+  }
+  if (CDB && !Cached)
+OnCommandChanged.broadcast(CDB->getAllFiles());
+  return CDB;
 }
 
 Optional
@@ -101,11 +107,14 @@
 
 void OverlayCDB::setCompileCommand(
 PathRef File, llvm::Optional Cmd) {
-  std::unique_lock Lock(Mutex);
-  if (Cmd)
-Commands[File] = std::move(*Cmd);
-  else
-Commands.erase(File);
+  {
+std::unique_lock Lock(Mutex);
+if (Cmd)
+  Commands[File] = std::move(*Cmd);
+else
+  Commands.erase(File);
+  }
+  OnCommandChanged.broadcast({File});
 }
 
 } // namespace clangd
Index: clangd/Function.h
===

r346756 - [clang-format] Do not treat the asm clobber [ as ObjCExpr

[Krasimir Georgiev via cfe-commits]

Author: krasimir
Date: Tue Nov 13 07:38:12 2018
New Revision: 346756

URL: http://llvm.org/viewvc/llvm-project?rev=346756&view=rev
Log:
[clang-format] Do not treat the asm clobber [ as ObjCExpr

Summary:
The opening square of an inline asm clobber was being annotated as an ObjCExpr.
This caused, amongst other things, the ObjCGuesser to guess header files
containing that pattern as ObjC files.

Reviewers: benhamilton

Reviewed By: benhamilton

Subscribers: cfe-commits

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

Modified:
cfe/trunk/lib/Format/TokenAnnotator.cpp
cfe/trunk/unittests/Format/FormatTest.cpp

Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=346756&r1=346755&r2=346756&view=diff
==
--- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp Tue Nov 13 07:38:12 2018
@@ -404,8 +404,9 @@ private:
 Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
 !CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
 (!Parent ||
- Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
- tok::kw_return, tok::kw_throw) ||
+ (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) ||
+ Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return,
+ tok::kw_throw) ||
  Parent->isUnaryOperator() ||
  // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
  Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||

Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=346756&r1=346755&r2=346756&view=diff
==
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Tue Nov 13 07:38:12 2018
@@ -12755,6 +12755,21 @@ TEST_F(FormatTest, GuessLanguageWithCare
   guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
 }
 
+TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+   "void f() {\n"
+   "  asm (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "void f() {\n"
+   "  asm volatile (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+}
+
 TEST_F(FormatTest, GuessLanguageWithChildLines) {
   EXPECT_EQ(FormatStyle::LK_Cpp,
 guessLanguage("foo.h", "#define FOO ({ std::string s; })"));


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

[PATCH] D54111: [clang-format] Do not treat the asm clobber [ as ObjCExpr

[Krasimir Georgiev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346756: [clang-format] Do not treat the asm clobber [ as 
ObjCExpr (authored by krasimir, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D54111

Files:
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp


Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -12755,6 +12755,21 @@
   guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
 }
 
+TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+   "void f() {\n"
+   "  asm (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "void f() {\n"
+   "  asm volatile (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+}
+
 TEST_F(FormatTest, GuessLanguageWithChildLines) {
   EXPECT_EQ(FormatStyle::LK_Cpp,
 guessLanguage("foo.h", "#define FOO ({ std::string s; })"));
Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -404,8 +404,9 @@
 Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
 !CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
 (!Parent ||
- Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
- tok::kw_return, tok::kw_throw) ||
+ (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) ||
+ Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return,
+ tok::kw_throw) ||
  Parent->isUnaryOperator() ||
  // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
  Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||


Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -12755,6 +12755,21 @@
   guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
 }
 
+TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+   "void f() {\n"
+   "  asm (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "void f() {\n"
+   "  asm volatile (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+}
+
 TEST_F(FormatTest, GuessLanguageWithChildLines) {
   EXPECT_EQ(FormatStyle::LK_Cpp,
 guessLanguage("foo.h", "#define FOO ({ std::string s; })"));
Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -404,8 +404,9 @@
 Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
 !CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
 (!Parent ||
- Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
- tok::kw_return, tok::kw_throw) ||
+ (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) ||
+ Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return,
+ tok::kw_throw) ||
  Parent->isUnaryOperator() ||
  // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
  Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 173856.
hokein marked 3 inline comments as done.
hokein added a comment.

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53427

Files:
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/Merge.cpp
  clangd/index/Serialization.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/YAMLSerialization.cpp
  clangd/index/dex/Dex.cpp
  unittests/clangd/BackgroundIndexTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/DexTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/SerializationTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -53,8 +53,10 @@
   return (arg.Name + arg.CompletionSnippetSuffix).str() == S;
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
-MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; }
+MATCHER_P(DeclURI, P, "") {
+  return StringRef(arg.CanonicalDeclaration.FileURI) == P;
+}
+MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
 MATCHER_P(IncludeHeader, P, "") {
   return (arg.IncludeHeaders.size() == 1) &&
  (arg.IncludeHeaders.begin()->IncludeHeader == P);
Index: unittests/clangd/SerializationTests.cpp
===
--- unittests/clangd/SerializationTests.cpp
+++ unittests/clangd/SerializationTests.cpp
@@ -111,7 +111,7 @@
   EXPECT_EQ(Sym1.Signature, "");
   EXPECT_EQ(Sym1.Documentation, "Foo doc");
   EXPECT_EQ(Sym1.ReturnType, "int");
-  EXPECT_EQ(Sym1.CanonicalDeclaration.FileURI, "file:///path/foo.h");
+  EXPECT_EQ(StringRef(Sym1.CanonicalDeclaration.FileURI), "file:///path/foo.h");
   EXPECT_EQ(Sym1.Origin, SymbolOrigin::Static);
   EXPECT_TRUE(Sym1.Flags & Symbol::IndexedForCodeCompletion);
   EXPECT_FALSE(Sym1.Flags & Symbol::Deprecated);
@@ -122,7 +122,8 @@
   EXPECT_THAT(Sym2, QName("clang::Foo2"));
   EXPECT_EQ(Sym2.Signature, "-sig");
   EXPECT_EQ(Sym2.ReturnType, "");
-  EXPECT_EQ(Sym2.CanonicalDeclaration.FileURI, "file:///path/bar.h");
+  EXPECT_EQ(llvm::StringRef(Sym2.CanonicalDeclaration.FileURI),
+"file:///path/bar.h");
   EXPECT_FALSE(Sym2.Flags & Symbol::IndexedForCodeCompletion);
   EXPECT_TRUE(Sym2.Flags & Symbol::Deprecated);
 
@@ -134,7 +135,7 @@
testing::SizeIs(1;
   auto Ref1 = ParsedYAML->Refs->begin()->second.front();
   EXPECT_EQ(Ref1.Kind, RefKind::Reference);
-  EXPECT_EQ(Ref1.Location.FileURI, "file:///path/foo.cc");
+  EXPECT_EQ(StringRef(Ref1.Location.FileURI), "file:///path/foo.cc");
 }
 
 std::vector YAMLFromSymbols(const SymbolSlab &Slab) {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -36,7 +36,7 @@
  std::make_tuple(Range.start.line, Range.start.character,
  Range.end.line, Range.end.character);
 }
-MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
+MATCHER_P(FileURI, F, "") { return StringRef(arg.Location.FileURI) == F; }
 
 TEST(SymbolLocation, Position) {
   using Position = SymbolLocation::Position;
@@ -222,7 +222,7 @@
 
   Symbol M = mergeSymbol(L, R);
   EXPECT_EQ(M.Name, "Foo");
-  EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:///left.h");
+  EXPECT_EQ(StringRef(M.CanonicalDeclaration.FileURI), "file:///left.h");
   EXPECT_EQ(M.References, 3u);
   EXPECT_EQ(M.Signature, "()");
   EXPECT_EQ(M.CompletionSnippetSuffix, "{$1:0}");
@@ -241,14 +241,14 @@
   R.Name = "right";
 
   Symbol M = mergeSymbol(L, R);
-  EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/left.h");
-  EXPECT_EQ(M.Definition.FileURI, "");
+  EXPECT_EQ(StringRef(M.CanonicalDeclaration.FileURI), "file:/left.h");
+  EXPECT_EQ(StringRef(M.Definition.FileURI), "");
   EXPECT_EQ(M.Name, "left");
 
   R.Definition.FileURI = "file:/right.cpp"; // Now right will be favored.
   M = mergeSymbol(L, R);
-  EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/right.h");
-  EXPECT_EQ(M.Definition.FileURI, "file:/right.cpp");
+  EXPECT_EQ(StringRef(M.CanonicalDeclaration.FileURI), "file:/right.h");
+  EXPECT_EQ(StringRef(M.Definition.FileURI), "file:/right.cpp");
   EXPECT_EQ(M.Name, "right");
 }
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -31,19 +31,21 @@
 using testing::IsEmpty;
 using testing::Pair;
 using testing::UnorderedElementsAre;
+using namespace llvm;
 
 MATCHER_P(RefRange, Range, "") {
   return std::make_tuple(arg.Location.Start.line(), arg.Location.S

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/index/Index.h:93
+return Cmp < 0;
+  return std::tie(L.Start, L.End) < std::tie(R.Start, R.End);
 }

sammccall wrote:
> tie(strcmp(L.FileURI, R.FileURI), ...) < tie(0, ...)?
tie doesn't support this usage, it expects lvalue arguments.



Comment at: clangd/index/Index.h:306
+llvm::StringRef S(P);
+CB(S);
+P = S.data();

sammccall wrote:
> ```assert (!S.data()[S.size()] && "Visited StringRef must be null-terminated")
Does this assert actually work? The `StringRef` is constructed from the `char 
*` (which using `strlen`). This assert will not get triggered I think, 
`S.data()[S.size()]` is always `\0`.



Comment at: clangd/index/Merge.cpp:118
   bool MergeIncludes =
-  L.Definition.FileURI.empty() == R.Definition.FileURI.empty();
+  !std::strlen(L.Definition.FileURI) == !std::strlen(R.Definition.FileURI);
   Symbol S = PreferR ? R : L;// The target symbol we're merging into.

sammccall wrote:
> Since this reads awkwardly anyway...
> ```bool(*L.Definition.FileURI) == bool(*R.definition.FileURI)```
> is shorter and faster
this is neat!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53427



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

[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/CodeGen/altivec-generic-overload.c:74
+  convert1(gv1);
+  // CHECK: call void @_Z8convert1Dv16_a(<16 x i8> %{{[0-9]+}})
+  convert1(gv2);

wuzish wrote:
> hubert.reinterpretcast wrote:
> > Checking that the call is to the expected target in terms of overload 
> > resolution can be achieved by having a distinct return types for each 
> > member of the overload set.
> What's meaning of `having a distinct return types for each member of the 
> overload set`?
> 
> Could you please give a example to show your expect?
This can be done with `-fsyntax-only`:
```
typedef signed char __v16sc __attribute__((__vector_size__(16)));
typedef unsigned char __v16uc __attribute__((__vector_size__(16)));

__v16sc *__attribute__((__overloadable__)) convert1(vector signed char);
__v16uc *__attribute__((__overloadable__)) convert1(vector unsigned char);

void test()
{
  __v16sc gv1;
  __v16uc gv2;
  _Generic(convert1(gv1), __v16sc *: (void)0);
  _Generic(convert1(gv2), __v16uc *: (void)0);
}
```



Comment at: clang/test/SemaCXX/vector.cpp:26
   float &fr1 = f1(ll16);
-  f1(c16e); // expected-error{{call to 'f1' is ambiguous}}
-  f1(ll16e); // expected-error{{call to 'f1' is ambiguous}}
+  f1(c16e);
+  f1(ll16e);

Check the return types here like with the two lines above.


https://reviews.llvm.org/D53417



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

r346757 - [NFC] Move storage of dispatch-version to GlobalDecl

[Erich Keane via cfe-commits]

Author: erichkeane
Date: Tue Nov 13 07:48:08 2018
New Revision: 346757

URL: http://llvm.org/viewvc/llvm-project?rev=346757&view=rev
Log:
[NFC] Move storage of dispatch-version to GlobalDecl

As suggested by Richard Smith, and initially put up for review here:
https://reviews.llvm.org/D53341, this patch removes a hack that was used
to ensure that proper target-feature lists were used when emitting
cpu-dispatch (and eventually, target-clones) implementations. As a part
of this, the GlobalDecl object is proliferated to a bunch more
locations.

Originally, this was put up for review (see above) to get acceptance on
the approach, though discussion with Richard in San Diego showed he
approved of the approach taken here.  Thus, I believe this is acceptable
for Review-After-commit

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

Change-Id: I0a0bd673340d334d93feac789d653e03d9f6b1d5

Modified:
cfe/trunk/include/clang/AST/GlobalDecl.h
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/lib/CodeGen/CGBlocks.cpp
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/CodeGen/CGCXX.cpp
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/CodeGen/CGCall.h
cfe/trunk/lib/CodeGen/CGClass.cpp
cfe/trunk/lib/CodeGen/CGException.cpp
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CGExprCXX.cpp
cfe/trunk/lib/CodeGen/CGNonTrivialStruct.cpp
cfe/trunk/lib/CodeGen/CGVTables.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/lib/Parse/ParseDeclCXX.cpp
cfe/trunk/test/CodeGen/attr-cpuspecific.c

Modified: cfe/trunk/include/clang/AST/GlobalDecl.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/GlobalDecl.h?rev=346757&r1=346756&r2=346757&view=diff
==
--- cfe/trunk/include/clang/AST/GlobalDecl.h (original)
+++ cfe/trunk/include/clang/AST/GlobalDecl.h Tue Nov 13 07:48:08 2018
@@ -34,6 +34,7 @@ namespace clang {
 /// a VarDecl, a FunctionDecl or a BlockDecl.
 class GlobalDecl {
   llvm::PointerIntPair Value;
+  unsigned MultiVersionIndex = 0;
 
   void Init(const Decl *D) {
 assert(!isa(D) && "Use other ctor with ctor decls!");
@@ -45,7 +46,10 @@ class GlobalDecl {
 public:
   GlobalDecl() = default;
   GlobalDecl(const VarDecl *D) { Init(D);}
-  GlobalDecl(const FunctionDecl *D) { Init(D); }
+  GlobalDecl(const FunctionDecl *D, unsigned MVIndex = 0)
+  : MultiVersionIndex(MVIndex) {
+Init(D);
+  }
   GlobalDecl(const BlockDecl *D) { Init(D); }
   GlobalDecl(const CapturedDecl *D) { Init(D); }
   GlobalDecl(const ObjCMethodDecl *D) { Init(D); }
@@ -57,6 +61,7 @@ public:
 GlobalDecl CanonGD;
 CanonGD.Value.setPointer(Value.getPointer()->getCanonicalDecl());
 CanonGD.Value.setInt(Value.getInt());
+CanonGD.MultiVersionIndex = MultiVersionIndex;
 
 return CanonGD;
   }
@@ -73,8 +78,17 @@ public:
 return static_cast(Value.getInt());
   }
 
+  unsigned getMultiVersionIndex() const {
+assert(isa(getDecl()) &&
+   !isa(getDecl()) &&
+   !isa(getDecl()) &&
+   "Decl is not a plain FunctionDecl!");
+return MultiVersionIndex;
+  }
+
   friend bool operator==(const GlobalDecl &LHS, const GlobalDecl &RHS) {
-return LHS.Value == RHS.Value;
+return LHS.Value == RHS.Value &&
+   LHS.MultiVersionIndex == RHS.MultiVersionIndex;
   }
 
   void *getAsOpaquePtr() const { return Value.getOpaqueValue(); }
@@ -90,6 +104,16 @@ public:
 Result.Value.setPointer(D);
 return Result;
   }
+
+  GlobalDecl getWithMultiVersionIndex(unsigned Index) {
+assert(isa(getDecl()) &&
+   !isa(getDecl()) &&
+   !isa(getDecl()) &&
+   "Decl is not a plain FunctionDecl!");
+GlobalDecl Result(*this);
+Result.MultiVersionIndex = Index;
+return Result;
+  }
 };
 
 } // namespace clang

Modified: cfe/trunk/include/clang/Basic/Attr.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=346757&r1=346756&r2=346757&view=diff
==
--- cfe/trunk/include/clang/Basic/Attr.td (original)
+++ cfe/trunk/include/clang/Basic/Attr.td Tue Nov 13 07:48:08 2018
@@ -864,10 +864,8 @@ def CPUSpecific : InheritableAttr {
   let Subjects = SubjectList<[Function]>;
   let Documentation = [CPUSpecificCPUDispatchDocs];
   let AdditionalMembers = [{
-unsigned ActiveArgIndex = 0;
-
-IdentifierInfo *getCurCPUName() const {
-  return *(cpus_begin() + ActiveArgIndex);
+IdentifierInfo *getCPUName(unsigned Index) const {
+  return *(cpus_begin() + Index);
 }
   }];
 }

Modified: cfe/trunk/lib/CodeGen/CGBlocks.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBlocks

[PATCH] D53341: Add MV-index to GlobalDecl, spread GlobalDecl through CodeGen (CPU-Dispatch cleanup)

[Erich Keane via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346757: [NFC] Move storage of dispatch-version to GlobalDecl 
(authored by erichkeane, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53341?vs=169892&id=173857#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53341

Files:
  cfe/trunk/include/clang/AST/GlobalDecl.h
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/lib/CodeGen/CGBlocks.cpp
  cfe/trunk/lib/CodeGen/CGBuiltin.cpp
  cfe/trunk/lib/CodeGen/CGCXX.cpp
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/CodeGen/CGCall.h
  cfe/trunk/lib/CodeGen/CGClass.cpp
  cfe/trunk/lib/CodeGen/CGException.cpp
  cfe/trunk/lib/CodeGen/CGExpr.cpp
  cfe/trunk/lib/CodeGen/CGExprCXX.cpp
  cfe/trunk/lib/CodeGen/CGNonTrivialStruct.cpp
  cfe/trunk/lib/CodeGen/CGVTables.cpp
  cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
  cfe/trunk/lib/CodeGen/CodeGenFunction.h
  cfe/trunk/lib/CodeGen/CodeGenModule.cpp
  cfe/trunk/lib/CodeGen/CodeGenModule.h
  cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
  cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
  cfe/trunk/lib/Parse/ParseDeclCXX.cpp
  cfe/trunk/test/CodeGen/attr-cpuspecific.c

Index: cfe/trunk/include/clang/AST/GlobalDecl.h
===
--- cfe/trunk/include/clang/AST/GlobalDecl.h
+++ cfe/trunk/include/clang/AST/GlobalDecl.h
@@ -34,6 +34,7 @@
 /// a VarDecl, a FunctionDecl or a BlockDecl.
 class GlobalDecl {
   llvm::PointerIntPair Value;
+  unsigned MultiVersionIndex = 0;
 
   void Init(const Decl *D) {
 assert(!isa(D) && "Use other ctor with ctor decls!");
@@ -45,7 +46,10 @@
 public:
   GlobalDecl() = default;
   GlobalDecl(const VarDecl *D) { Init(D);}
-  GlobalDecl(const FunctionDecl *D) { Init(D); }
+  GlobalDecl(const FunctionDecl *D, unsigned MVIndex = 0)
+  : MultiVersionIndex(MVIndex) {
+Init(D);
+  }
   GlobalDecl(const BlockDecl *D) { Init(D); }
   GlobalDecl(const CapturedDecl *D) { Init(D); }
   GlobalDecl(const ObjCMethodDecl *D) { Init(D); }
@@ -57,6 +61,7 @@
 GlobalDecl CanonGD;
 CanonGD.Value.setPointer(Value.getPointer()->getCanonicalDecl());
 CanonGD.Value.setInt(Value.getInt());
+CanonGD.MultiVersionIndex = MultiVersionIndex;
 
 return CanonGD;
   }
@@ -73,8 +78,17 @@
 return static_cast(Value.getInt());
   }
 
+  unsigned getMultiVersionIndex() const {
+assert(isa(getDecl()) &&
+   !isa(getDecl()) &&
+   !isa(getDecl()) &&
+   "Decl is not a plain FunctionDecl!");
+return MultiVersionIndex;
+  }
+
   friend bool operator==(const GlobalDecl &LHS, const GlobalDecl &RHS) {
-return LHS.Value == RHS.Value;
+return LHS.Value == RHS.Value &&
+   LHS.MultiVersionIndex == RHS.MultiVersionIndex;
   }
 
   void *getAsOpaquePtr() const { return Value.getOpaqueValue(); }
@@ -90,6 +104,16 @@
 Result.Value.setPointer(D);
 return Result;
   }
+
+  GlobalDecl getWithMultiVersionIndex(unsigned Index) {
+assert(isa(getDecl()) &&
+   !isa(getDecl()) &&
+   !isa(getDecl()) &&
+   "Decl is not a plain FunctionDecl!");
+GlobalDecl Result(*this);
+Result.MultiVersionIndex = Index;
+return Result;
+  }
 };
 
 } // namespace clang
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -864,10 +864,8 @@
   let Subjects = SubjectList<[Function]>;
   let Documentation = [CPUSpecificCPUDispatchDocs];
   let AdditionalMembers = [{
-unsigned ActiveArgIndex = 0;
-
-IdentifierInfo *getCurCPUName() const {
-  return *(cpus_begin() + ActiveArgIndex);
+IdentifierInfo *getCPUName(unsigned Index) const {
+  return *(cpus_begin() + Index);
 }
   }];
 }
Index: cfe/trunk/test/CodeGen/attr-cpuspecific.c
===
--- cfe/trunk/test/CodeGen/attr-cpuspecific.c
+++ cfe/trunk/test/CodeGen/attr-cpuspecific.c
@@ -223,6 +223,34 @@
 // WINDOWS-NOT: call i32 @GenericAndPentium.A
 // WINDOWS-NOT: call void @llvm.trap
 
+ATTR(cpu_dispatch(atom, pentium))
+int DispatchFirst(void);
+// LINUX: define i32 ()* @DispatchFirst.resolver
+// LINUX: ret i32 ()* @DispatchFirst.O
+// LINUX: ret i32 ()* @DispatchFirst.B
+
+// WINDOWS: define dso_local i32 @DispatchFirst()
+// WINDOWS: %[[RET:.+]] = musttail call i32 @DispatchFirst.O()
+// WINDOWS-NEXT: ret i32 %[[RET]]
+// WINDOWS: %[[RET:.+]] = musttail call i32 @DispatchFirst.B()
+// WINDOWS-NEXT: ret i32 %[[RET]]
+
+ATTR(cpu_specific(atom))
+int DispatchFirst(void) {return 0;}
+// LINUX: define i32 @DispatchFirst.O
+// LINUX: ret i32 0
+
+// WINDOWS: define dso_local i32 @DispatchFirst.O()
+// WINDOWS: ret i32 0
+
+ATTR(cpu_specific(pentium))
+int DispatchFirst(void) {return 1;}
+// LINUX: 

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/index/Index.h:306
+llvm::StringRef S(P);
+CB(S);
+P = S.data();

hokein wrote:
> sammccall wrote:
> > ```assert (!S.data()[S.size()] && "Visited StringRef must be 
> > null-terminated")
> Does this assert actually work? The `StringRef` is constructed from the `char 
> *` (which using `strlen`). This assert will not get triggered I think, 
> `S.data()[S.size()]` is always `\0`.
The point is the callback takes a mutable reference, and can replace the 
stringref.
We're asserting that the *new* stringref is nullterminated (so we can convert 
it safely to a `char*`)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53427



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

[PATCH] D54269: Introduce shard storage to auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 173859.
kadircet marked 13 inline comments as done.
kadircet added a comment.

- Address comments.
- Deleted shard loading logic from backgroundindex.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269

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

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -76,5 +76,68 @@
FileURI("unittest:///root/B.cc")}));
 }
 
+TEST(BackgroundIndexTest, ShardStorageWriteTest) {
+  class MemoryShardStorage : public BackgroundIndexStorage {
+mutable std::mutex StorageMu;
+llvm::StringMap &Storage;
+size_t &CacheHits;
+
+  public:
+MemoryShardStorage(llvm::StringMap &Storage, size_t &CacheHits)
+: Storage(Storage), CacheHits(CacheHits) {}
+llvm::Error storeShard(llvm::StringRef ShardIdentifier,
+   IndexFileOut Shard) const override {
+  std::lock_guard Lock(StorageMu);
+  std::string &str = Storage[ShardIdentifier];
+  llvm::raw_string_ostream OS(str);
+  OS << Shard;
+  OS.flush();
+  return llvm::Error::success();
+}
+llvm::Expected
+loadShard(llvm::StringRef ShardIdentifier) const override {
+  std::lock_guard Lock(StorageMu);
+  if (Storage.find(ShardIdentifier) == Storage.end())
+return llvm::make_error(
+"Shard not found.", llvm::inconvertibleErrorCode());
+  auto IndexFile = readIndexFile(Storage[ShardIdentifier]);
+  if (!IndexFile)
+return IndexFile;
+  CacheHits++;
+  return IndexFile;
+}
+  };
+  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;
+  BackgroundIndex::IndexStorageFactory MemoryStorageFactory =
+  [&Storage, &CacheHits](llvm::StringRef) {
+return llvm::make_unique(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.
+  {
+BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"},
+MemoryStorageFactory);
+Idx.enqueue(testPath("root"), Cmd);
+Idx.blockUntilIdleForTest();
+  }
+  EXPECT_EQ(Storage.size(), 2U);
+  EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
+  EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -14,6 +14,7 @@
 #include "FSProvider.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
+#include "index/Serialization.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/SHA1.h"
@@ -27,15 +28,35 @@
 namespace clang {
 namespace clangd {
 
+// Handles storage and retrieval of index shards.
+class BackgroundIndexStorage {
+public:
+  // Stores given shard associationg with ShardIdentifier, which can be
+  // retrieved later on with the same identifier.
+  virtual llvm::Error storeShard(llvm::StringRef ShardIdentifier,
+ IndexFileOut Shard) const = 0;
+
+  virtual llvm::Expected
+  loadShard(llvm::StringRef ShardIdentifier) const = 0;
+
+  static std::unique_ptr
+  createDiskStorage(llvm::StringRef CDBDirectory);
+};
+
 // Builds an in-memory index by by running the static indexer action over
 // all commands in a compilation database. Indexing happens in the background.
+// Takes a factory function to create IndexStorage units for each compilation
+// database. Those databases are identified by directory they are found.
 // FIXME: it should also persist its state on disk for fast start.
 // FIXME: it should watch for changes to files on disk.
 class BackgroundIndex : public SwapIndex {
 public:
+  using IndexStorageFactory =
+  std::function(llvm::StringRef)>;
   // FIXME: resource-dir injection should be hoisted somewhere common.
-  BackgroundIndex(Context BackgroundContext, StringRef ResourceDir,
+  BackgroundIndex(Context BackgroundContext, llvm::StringRef ResourceDir,
   const FileSystemProvider &, ArrayRef URISchemes,
+  IndexStorageFactory IndexStorageCreator = nullptr,
   size_t ThreadPoolSize = llvm::hardware_concurrency());
   ~BackgroundIndex(); // Blocks while the current task finishes.
 
@@ -59,25 +8

[PATCH] D54269: Introduce shard storage to auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/index/Background.cpp:52
+llvm::SmallString<128>
+getShardPathFromFilePath(llvm::SmallString<128> ShardRoot,
+ llvm::StringRef FilePath) {

sammccall wrote:
> nit: these micro-optimizations with SmallString seem unlikely to be 
> worthwhile - maybe just take StringRef and return string?
It wasn't for optimization purposes actually, sys::path::append only accepts a 
SmallString and there is no direct conversion between SmallString and 
std::string(or stringref).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[Arthur O'Dwyer via Phabricator via cfe-commits]

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



Comment at: docs/LanguageExtensions.rst:1093
   library.
+* ``__is_trivially_relocatable`` (Clang): Determines whether moving an object
+  of type ``type``, and then destroying the source object, is functionally

erichkeane wrote:
> How would this behave with unions?  I don't see any exclusions happening on 
> union-ness.  A CXXRecordDecl can be a union as well as a class.
My intent is to make Clang's behavior match the wording in P1144R0; so, it 
should work for unions as well as for structs/classes. Any union which is 
trivially move-constructible and trivially destructible will be 
`__is_trivially_relocatable`. Any union which is non-trivially destructible 
*must* have a user-provided destructor, and therefore will *not* be 
`__is_trivially_relocatable` unless the user has annotated it with the 
attribute.
https://p1144.godbolt.org/z/F06TTQ



Comment at: include/clang/AST/DeclCXX.h:482
+/// and a defaulted destructor.
+unsigned IsNaturallyTriviallyRelocatable : 1;
+

erichkeane wrote:
> Typically we'd have this calculated when requested rather than stored. I 
> suspect using a bit for something like this isn't going to be terribly 
> acceptable.
You know better than I do; but I'm also not sure how to calculate it on request.



Comment at: include/clang/Basic/Attr.td:2096
+def TriviallyRelocatable : InheritableAttr {
+  let Spellings = [CXX11<"", "trivially_relocatable", 200809>,
+   CXX11<"clang", "trivially_relocatable">];

erichkeane wrote:
> This spelling is almost definitely not acceptable until it is in the 
> standard.  Also, why specify that it was added in 2008?
Agreed, it should be `[[clang::trivially_relocatable]]` for Clang's purposes. 
This spelling was because this patch came from the Godbolt Compiler Explorer 
patch where I wanted the shorter/future spelling for public relations reasons. 
:)
IIUC, the appropriate fix here is to change these two lines from
```
let Spellings = [CXX11<"", "trivially_relocatable", "200809">,
  CXX11<"clang", "trivially_relocatable">];
```
to
```
let Spellings = [Clang<"trivially_relocatable">,
```
I believe the "200809" was because I wanted it to be available in C++11 mode on 
Godbolt.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8207
 
+def err_attribute_missing_on_first_decl : Error<
+  "type %0 declared %1 after its first declaration">;

erichkeane wrote:
> I'm shocked that there isn't a different diagnostic to do this same thing.  
> @aaron.ballman likely knows better...  I haven't seen the usage yet, but I 
> presume you don't necessarily want a behavior that doesn't allow forward 
> declarations.
I would be very happy to see this diagnostic get into trunk separately and 
earlier than D50119. There are some other diagnostics that could be merged with 
this one, e.g. `[[noreturn]]` needs a version of this diagnostic, and I believe 
`[[clang::trivial_abi]]` should have it added.

I don't know how to link to comments on Phabricator, but Ctrl+F downward for 
this example:
```
struct S { S(S&&); ~S(); };
std::vector vec;
struct [[trivially_relocatable]] S;  // ha ha, now you have to re-do all of 
vector's codegen!
```
This is why it is important to diagnose and disallow "backward declarations." I 
don't particularly care about "forward declarations" (either to allow or 
disallow them). The attribute would end up getting used only on library types 
where IMHO nobody should ever be forward-declaring them anyway. E.g. it is not 
a good idea for a regular C++ programmer to forward-declare `unique_ptr`. But 
if there's a way to allow forward-declarations (when the type remains 
incomplete) while disallowing backward-declarations (adding the attribute to an 
already-complete type), then I will be happy to do it.



Comment at: lib/AST/Type.cpp:2234
+bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
+  QualType T = Context.getBaseElementType(*this);
+  if (T->isIncompleteType())

erichkeane wrote:
> You likely want to canonicalize here.
You mean `QualType T = Context.getBaseElementType(getCanonicalType());`?
I can do that. For my own edification (and/or a test case), in what way does 
the current code fail?


Repository:
  rC Clang

https://reviews.llvm.org/D50119



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

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 173861.
hokein marked an inline comment as done.
hokein added a comment.

Add assert.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53427

Files:
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/Merge.cpp
  clangd/index/Serialization.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/YAMLSerialization.cpp
  clangd/index/dex/Dex.cpp
  unittests/clangd/BackgroundIndexTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/DexTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/SerializationTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -53,8 +53,10 @@
   return (arg.Name + arg.CompletionSnippetSuffix).str() == S;
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
-MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
-MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; }
+MATCHER_P(DeclURI, P, "") {
+  return StringRef(arg.CanonicalDeclaration.FileURI) == P;
+}
+MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
 MATCHER_P(IncludeHeader, P, "") {
   return (arg.IncludeHeaders.size() == 1) &&
  (arg.IncludeHeaders.begin()->IncludeHeader == P);
Index: unittests/clangd/SerializationTests.cpp
===
--- unittests/clangd/SerializationTests.cpp
+++ unittests/clangd/SerializationTests.cpp
@@ -111,7 +111,7 @@
   EXPECT_EQ(Sym1.Signature, "");
   EXPECT_EQ(Sym1.Documentation, "Foo doc");
   EXPECT_EQ(Sym1.ReturnType, "int");
-  EXPECT_EQ(Sym1.CanonicalDeclaration.FileURI, "file:///path/foo.h");
+  EXPECT_EQ(StringRef(Sym1.CanonicalDeclaration.FileURI), "file:///path/foo.h");
   EXPECT_EQ(Sym1.Origin, SymbolOrigin::Static);
   EXPECT_TRUE(Sym1.Flags & Symbol::IndexedForCodeCompletion);
   EXPECT_FALSE(Sym1.Flags & Symbol::Deprecated);
@@ -122,7 +122,8 @@
   EXPECT_THAT(Sym2, QName("clang::Foo2"));
   EXPECT_EQ(Sym2.Signature, "-sig");
   EXPECT_EQ(Sym2.ReturnType, "");
-  EXPECT_EQ(Sym2.CanonicalDeclaration.FileURI, "file:///path/bar.h");
+  EXPECT_EQ(llvm::StringRef(Sym2.CanonicalDeclaration.FileURI),
+"file:///path/bar.h");
   EXPECT_FALSE(Sym2.Flags & Symbol::IndexedForCodeCompletion);
   EXPECT_TRUE(Sym2.Flags & Symbol::Deprecated);
 
@@ -134,7 +135,7 @@
testing::SizeIs(1;
   auto Ref1 = ParsedYAML->Refs->begin()->second.front();
   EXPECT_EQ(Ref1.Kind, RefKind::Reference);
-  EXPECT_EQ(Ref1.Location.FileURI, "file:///path/foo.cc");
+  EXPECT_EQ(StringRef(Ref1.Location.FileURI), "file:///path/foo.cc");
 }
 
 std::vector YAMLFromSymbols(const SymbolSlab &Slab) {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -36,7 +36,7 @@
  std::make_tuple(Range.start.line, Range.start.character,
  Range.end.line, Range.end.character);
 }
-MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
+MATCHER_P(FileURI, F, "") { return StringRef(arg.Location.FileURI) == F; }
 
 TEST(SymbolLocation, Position) {
   using Position = SymbolLocation::Position;
@@ -222,7 +222,7 @@
 
   Symbol M = mergeSymbol(L, R);
   EXPECT_EQ(M.Name, "Foo");
-  EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:///left.h");
+  EXPECT_EQ(StringRef(M.CanonicalDeclaration.FileURI), "file:///left.h");
   EXPECT_EQ(M.References, 3u);
   EXPECT_EQ(M.Signature, "()");
   EXPECT_EQ(M.CompletionSnippetSuffix, "{$1:0}");
@@ -241,14 +241,14 @@
   R.Name = "right";
 
   Symbol M = mergeSymbol(L, R);
-  EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/left.h");
-  EXPECT_EQ(M.Definition.FileURI, "");
+  EXPECT_EQ(StringRef(M.CanonicalDeclaration.FileURI), "file:/left.h");
+  EXPECT_EQ(StringRef(M.Definition.FileURI), "");
   EXPECT_EQ(M.Name, "left");
 
   R.Definition.FileURI = "file:/right.cpp"; // Now right will be favored.
   M = mergeSymbol(L, R);
-  EXPECT_EQ(M.CanonicalDeclaration.FileURI, "file:/right.h");
-  EXPECT_EQ(M.Definition.FileURI, "file:/right.cpp");
+  EXPECT_EQ(StringRef(M.CanonicalDeclaration.FileURI), "file:/right.h");
+  EXPECT_EQ(StringRef(M.Definition.FileURI), "file:/right.cpp");
   EXPECT_EQ(M.Name, "right");
 }
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -31,19 +31,21 @@
 using testing::IsEmpty;
 using testing::Pair;
 using testing::UnorderedElementsAre;
+using namespace llvm;
 
 MATCHER_P(RefRange, Range, "") {
   return std::make_tuple(arg.Location.Start.line(), arg.Location.Start.column()

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/index/Index.h:306
+llvm::StringRef S(P);
+CB(S);
+P = S.data();

sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > ```assert (!S.data()[S.size()] && "Visited StringRef must be 
> > > null-terminated")
> > Does this assert actually work? The `StringRef` is constructed from the 
> > `char *` (which using `strlen`). This assert will not get triggered I 
> > think, `S.data()[S.size()]` is always `\0`.
> The point is the callback takes a mutable reference, and can replace the 
> stringref.
> We're asserting that the *new* stringref is nullterminated (so we can convert 
> it safely to a `char*`)
Ah, that makes sense, I missed the fact that the `CB` mutates `S`. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53427



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Hey @rjmccall - I'm trying to remember, but can't get it quite clear in my 
head. I seem to recall some discussion about trivial_abi not implying/being 
strong enough for all the cases that trivial_relocatable sounds like it covers? 
Do you happen to remember the distinction there (the summary in this patch 
("the warranting attribute [[trivially_relocatable]], which is similar in 
spirit to [[trivial_abi]], in that it lets the programmer communicate back to 
the compiler that a certain user-defined type should be assumed to have this 
property even though it would not naturally have the property all else being 
equal.") doesn't sound like what I remember - but my memory is hazy)?


Repository:
  rC Clang

https://reviews.llvm.org/D50119



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: docs/LanguageExtensions.rst:1093
   library.
+* ``__is_trivially_relocatable`` (Clang): Determines whether moving an object
+  of type ``type``, and then destroying the source object, is functionally

Quuxplusone wrote:
> erichkeane wrote:
> > How would this behave with unions?  I don't see any exclusions happening on 
> > union-ness.  A CXXRecordDecl can be a union as well as a class.
> My intent is to make Clang's behavior match the wording in P1144R0; so, it 
> should work for unions as well as for structs/classes. Any union which is 
> trivially move-constructible and trivially destructible will be 
> `__is_trivially_relocatable`. Any union which is non-trivially destructible 
> *must* have a user-provided destructor, and therefore will *not* be 
> `__is_trivially_relocatable` unless the user has annotated it with the 
> attribute.
> https://p1144.godbolt.org/z/F06TTQ
Makes sense.



Comment at: include/clang/AST/DeclCXX.h:482
+/// and a defaulted destructor.
+unsigned IsNaturallyTriviallyRelocatable : 1;
+

Quuxplusone wrote:
> erichkeane wrote:
> > Typically we'd have this calculated when requested rather than stored. I 
> > suspect using a bit for something like this isn't going to be terribly 
> > acceptable.
> You know better than I do; but I'm also not sure how to calculate it on 
> request.
You'll end up just recursively walking the CXXRecordDecl upon application of 
the type trait, and need to check the things that make it NOT trivially 
relocatable.

I think it also extensively simplifies this patch, since none of the 
CXXRecordDecl functions are required (just the type-trait emission).



Comment at: include/clang/Basic/Attr.td:2096
+def TriviallyRelocatable : InheritableAttr {
+  let Spellings = [CXX11<"", "trivially_relocatable", 200809>,
+   CXX11<"clang", "trivially_relocatable">];

Quuxplusone wrote:
> erichkeane wrote:
> > This spelling is almost definitely not acceptable until it is in the 
> > standard.  Also, why specify that it was added in 2008?
> Agreed, it should be `[[clang::trivially_relocatable]]` for Clang's purposes. 
> This spelling was because this patch came from the Godbolt Compiler Explorer 
> patch where I wanted the shorter/future spelling for public relations 
> reasons. :)
> IIUC, the appropriate fix here is to change these two lines from
> ```
> let Spellings = [CXX11<"", "trivially_relocatable", "200809">,
>   CXX11<"clang", "trivially_relocatable">];
> ```
> to
> ```
> let Spellings = [Clang<"trivially_relocatable">,
> ```
> I believe the "200809" was because I wanted it to be available in C++11 mode 
> on Godbolt.
Yep, thats the suggested fix.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8207
 
+def err_attribute_missing_on_first_decl : Error<
+  "type %0 declared %1 after its first declaration">;

Quuxplusone wrote:
> erichkeane wrote:
> > I'm shocked that there isn't a different diagnostic to do this same thing.  
> > @aaron.ballman likely knows better...  I haven't seen the usage yet, but I 
> > presume you don't necessarily want a behavior that doesn't allow forward 
> > declarations.
> I would be very happy to see this diagnostic get into trunk separately and 
> earlier than D50119. There are some other diagnostics that could be merged 
> with this one, e.g. `[[noreturn]]` needs a version of this diagnostic, and I 
> believe `[[clang::trivial_abi]]` should have it added.
> 
> I don't know how to link to comments on Phabricator, but Ctrl+F downward for 
> this example:
> ```
> struct S { S(S&&); ~S(); };
> std::vector vec;
> struct [[trivially_relocatable]] S;  // ha ha, now you have to re-do all of 
> vector's codegen!
> ```
> This is why it is important to diagnose and disallow "backward declarations." 
> I don't particularly care about "forward declarations" (either to allow or 
> disallow them). The attribute would end up getting used only on library types 
> where IMHO nobody should ever be forward-declaring them anyway. E.g. it is 
> not a good idea for a regular C++ programmer to forward-declare `unique_ptr`. 
> But if there's a way to allow forward-declarations (when the type remains 
> incomplete) while disallowing backward-declarations (adding the attribute to 
> an already-complete type), then I will be happy to do it.
Sure, I get that... I'm just surprised/amazed it doesn't already exist.  
Hopefully Aaron takes a look.



Comment at: lib/AST/Type.cpp:2234
+bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
+  QualType T = Context.getBaseElementType(*this);
+  if (T->isIncompleteType())

Quuxplusone wrote:
> erichkeane wrote:
> > You likely want to canonicalize here.
> You mean `QualType T = Context.getBaseElementType(getCanonicalType());`?
> I can do that. For my own

[PATCH] D51762: First part of the calendar stuff

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

Can this be closed? My understanding is that this has been committed now.


https://reviews.llvm.org/D51762



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

[PATCH] D52418: [driver][mips] Enable integrated assembler for MIPS64 except N32 ABI selected

[Simon Atanasyan via Phabricator via cfe-commits]

atanasyan added a comment.

In https://reviews.llvm.org/D52418#1294699, @brad wrote:

> Simon?


The testing continues. Unfortunately I do not have access to an appropriate 
hardware so cannot control the process. But the task is not abandoned.


Repository:
  rC Clang

https://reviews.llvm.org/D52418



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D50119#1297070, @dblaikie wrote:

> Hey @rjmccall - I'm trying to remember, but can't get it quite clear in my 
> head. I seem to recall some discussion about trivial_abi not implying/being 
> strong enough for all the cases that trivial_relocatable sounds like it 
> covers? Do you happen to remember the distinction there (the summary in this 
> patch ("the warranting attribute [[trivially_relocatable]], which is similar 
> in spirit to [[trivial_abi]], in that it lets the programmer communicate back 
> to the compiler that a certain user-defined type should be assumed to have 
> this property even though it would not naturally have the property all else 
> being equal.") doesn't sound like what I remember - but my memory is hazy)?


`trivial_abi` permits annotated types to be passed and returned in registers, 
which is ABI-breaking.  Skimming the blog post, it looks like 
`trivially_relocatable` does not permit this — it merely signifies that 
destruction is a no-op after a move construction or assignment.  This is 
usefully different in the design space, since it means you can safely add the 
attribute retroactively to e.g. `std::unique_ptr`, and other templates can then 
detect that `std::unique_ptr` is trivially-relocatable and optimize themselves 
to use `memcpy` or `realloc` or whatever it is that they want to do.  So in 
that sense `trivial_abi` is a *stronger* attribute, not a *weaker* one: the 
property it determines ought to imply `trivially_relocatable`.

The only interesting question in the language design that I know of is what 
happens if you put the attribute on a template that's instantiated to contain a 
sub-object that is definitely not trivially relocatable / trivial-ABI.  For 
`trivial_abi`, we decided that the attribute is simply ignored — it implicitly 
only applies to specializations where the attribute would be legal.  I haven't 
dug into the design enough to know what `trivially_relocatable` decides in this 
situation, but the three basic options are:

- the attribute always has effect and allows trivial relocation regardless of 
the subobject types; this is obviously unsafe, so it limits the safe 
applicability of the attribute to templates
- the attribute is ignored, like `trivial_abi` is
- the attribute is ill-formed, and you'll need to add a 
`[[trivially_relocatable(bool)]]` version to support templates

If there are corner-case differences beyond that, I have no particular 
objection to unifying the semantics so that `trivial_abi` is strictly stronger, 
although we should talk about it case-by-case.




Comment at: test/SemaCXX/trivially-relocatable.cpp:42
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error@-1{{type A6 declared 'trivially_relocatable' after its first 
declaration}}

Rakete wrote:
> Quuxplusone wrote:
> > Rakete wrote:
> > > Why does this restriction exist? None of the existing attributes have it 
> > > and I don't see why it would make sense to disallow this.
> > `[[noreturn]]` has it, and I am pretty sure that `[[trivial_abi]]` *ought* 
> > to have it, even though it currently doesn't. The intent is to make it 
> > harder for people to create ODR violations by declaring a type, using it in 
> > some way, and then saying "oh by the way this type was x all along."
> > 
> > ```
> > struct S { S(S&&); ~S(); };
> > std::vector vec;
> > struct [[trivially_relocatable]] S;  // ha ha, now you have to re-do all of 
> > vector's codegen!
> > ```
> > 
> > Does that make sense as a reason it would be (mildly) beneficial to have 
> > this diagnostic?
> > You could still reasonably object to my assumptions that (A) the diagnostic 
> > is worth implementing in Clang and/or (B) the diagnostic is worth mandating 
> > in the Standard wording; but FWIW I do think it's very cheap both to 
> > implement and to specify.
> > 
> > (I will try to adjust the patch so that after the error is given, we remove 
> > the attribute from the class so as to make consistent any subsequent 
> > cascading errors. https://godbolt.org/g/nVRiow )
> Didn't know `[[noreturn]]` had it. Ah I see. Sold :)
I don't see any good reason to allow either this or `[[trivial_abi]]` on an 
arbitrary declaration of the type; it should have to be written on the 
definition.  This also gives sensible semantics for class template 
specializations.

The intrinsic obviously has to require its operand type to be complete, and 
therefore there is a unique declaration which can have the attribute.


Repository:
  rC Clang

https://reviews.llvm.org/D50119



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

[PATCH] D54428: [clangd] XPC transport layer, framework, test-client

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

A question about the high-level build target setup (I don't know much about XPC 
services/frameworks, bear with me...):

This is set up so that the clangd binary (ClangdMain) can run unmodified as an 
XPC service, all flags and options are still respected etc.
At the same time, the framework plist seems like it is designed to invoke the 
clangd binary in a very specific configuration (no flags will be plumbed 
through).

Is it actually important that the `clangd` binary itself can be used with XPC, 
rather than defining another binary that spawns a `ClangdServer+XPCTransport` 
with the right config? If you strip out all of `ClangdMain` that's not related 
to flag parsing and options, there's almost nothing left...




Comment at: tool/ClangdMain.cpp:30
+#ifdef CLANGD_BUILD_XPC
+#include "xpc/XPCTransport.h"
+#endif

WDYT about putting `newXPCTransport()` in `Transport.h` behind an ifdef?

That will be consistent with JSON, allow the `XPCTransport.h` to be removed 
entirely, and remove one #ifdef here.

I find the #ifdefs easier to understand if they surround functional code, 
rather than #includes - might be just me.



Comment at: tool/ClangdMain.cpp:328
+#ifdef CLANGD_BUILD_XPC
+if (getenv("CLANGD_AS_XPC_SERVICE"))
+  return newXPCransport();

I'd consider putting this outside the #ifdef, so we can print an error and exit 
if it's requested but not built.

In fact, can this just be a regular flag instead? `-transport={xpc|json}`



Comment at: tool/ClangdMain.cpp:329
+if (getenv("CLANGD_AS_XPC_SERVICE"))
+  return newXPCransport();
+#endif

no support for `-input-mirror` or pretty-printing in the logs - deliberate?



Comment at: xpc/CMakeLists.txt:21
+
+add_clang_library(clangdXpcJsonConversions
+  Conversion.cpp

why is conversions a separate library from transport?

No problem with fine-grained targets per se, but it's inconsistent with much of 
the rest of clang-tools-extra.



Comment at: xpc/Conversion.cpp:16
+
+using namespace clang;
+using namespace clangd;

nit:
```
using namespace llvm;
namespace clang {
namespace clangd {
```
(we're finally consistent about this)



Comment at: xpc/Conversion.cpp:22
+xpc_object_t clangd::jsonToXpc(const json::Value &json) {
+  const char *const key = "LSP";
+  std::string payload_string;

Key, PayloadString, etc



Comment at: xpc/Conversion.cpp:23
+  const char *const key = "LSP";
+  std::string payload_string;
+  raw_string_ostream payload_stream(payload_string);

nit: you can #include ScopedPrinter, and then this is just llvm::to_string(json)



Comment at: xpc/Conversion.cpp:28
+  xpc_object_t payload_obj = xpc_string_create(payload_string.c_str());
+  return xpc_dictionary_create(&key, &payload_obj, 1);
+}

ah, this encoding is a little sad vs the "native" encoding of object -> 
xpc_dictionary, array -> xpc_array etc which looked promising...

Totally your call, but out of curiosity - why the change?



Comment at: xpc/Conversion.h:19
+
+xpc_object_t jsonToXpc(const llvm::json::Value &json);
+llvm::json::Value xpcToJson(const xpc_object_t &json);

param name json -> JSON



Comment at: xpc/XPCTransport.cpp:142
+
+namespace XPCClosure {
+// "owner" of this "closure object" - necessary for propagating connection to

lowercase namespace?



Comment at: xpc/XPCTransport.cpp:177
+if (!TransportObject->handleMessage(std::move(Doc), *HandlerPtr)) {
+  log("Received exit notification - cancelling connection.");
+  xpc_connection_cancel(xpc_dictionary_get_remote_connection(message));

I'm not clear on the relationship between XPC services and connections - if 
there are multiple connections, what happens? Do you want to check and close 
any subsequent ones? Can connections and/or messages arrive on multiple threads?



Comment at: xpc/XPCTransport.cpp:192
+  // exit of xpc_main().
+  XPCClosure::TransportObject = this;
+  XPCClosure::HandlerPtr = &Handler;

you could consider asserting that they were previously null



Comment at: xpc/XPCTransport.h:19
+
+std::unique_ptr newXPCransport();
+

as mentioned above, I think this may be more discoverable in Transport.h, with 
ifdef.
But if the layering/separate libraries are important, here seems OK too.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54428



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

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

[Anastasia Stulova via Phabricator via cfe-commits]

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

Rewrite how CastKind is set for reference and pointer type.


https://reviews.llvm.org/D53764

Files:
  include/clang/Sema/Sema.h
  lib/AST/Expr.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Sema/DeclSpec.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/address-space-deduction.cl

Index: test/CodeGenOpenCLCXX/address-space-deduction.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/address-space-deduction.cl
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s -check-prefixes=COMMON,PTR
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - -DREF | FileCheck %s -check-prefixes=COMMON,REF
+
+#ifdef REF
+#define PTR &
+#define ADR(x) x
+#else
+#define PTR *
+#define ADR(x) &x
+#endif
+
+//COMMON: @glob = addrspace(1) global i32
+int glob;
+//PTR: @glob_p = addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*)
+//REF: @glob_p = addrspace(1) global i32 addrspace(4)* null
+int PTR glob_p = ADR(glob);
+
+//COMMON: @_ZZ3fooi{{P|R}}U3AS4iE6loc_st = internal addrspace(1) global i32
+//PTR: @_ZZ3fooiPU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiPU3AS4iE6loc_st to i32 addrspace(4)*)
+//REF: @_ZZ3fooiRU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* null
+//COMMON: @loc_ext_p = external addrspace(1) {{global|constant}} i32 addrspace(4)*
+//COMMON: @loc_ext = external addrspace(1) global i32
+
+//REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @glob_p
+
+//COMMON: define spir_func i32 @_Z3fooi{{P|R}}U3AS4i(i32 %par, i32 addrspace(4)*{{.*}} %par_p)
+int foo(int par, int PTR par_p){
+//COMMON: %loc = alloca i32
+  int loc;
+//COMMON: %loc_p = alloca i32 addrspace(4)*
+//COMMON: [[GAS:%[0-9]+]] = addrspacecast i32* %loc to i32 addrspace(4)*
+//COMMON: store i32 addrspace(4)* [[GAS]], i32 addrspace(4)** %loc_p
+  int PTR loc_p = ADR(loc);
+
+// CHECK directives for the following code are located above.
+  static int loc_st;
+  static int PTR loc_st_p = ADR(loc_st);
+  extern int loc_ext;
+  extern int PTR loc_ext_p;
+  (void)loc_ext_p;
+  return loc_ext;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7177,7 +7177,8 @@
   bool IsPointee =
   ChunkIndex > 0 &&
   (D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Pointer ||
-   D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer);
+   D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer ||
+   D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Reference);
   bool IsFuncReturnType =
   ChunkIndex > 0 &&
   D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Function;
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -7209,12 +7209,20 @@
   return CreateMaterializeTemporaryExpr(E->getType(), E, false);
 }
 
-ExprResult
-InitializationSequence::Perform(Sema &S,
-const InitializedEntity &Entity,
-const InitializationKind &Kind,
-MultiExprArg Args,
-QualType *ResultType) {
+ExprResult Sema::PerformQualificationConversion(Expr *E, QualType Ty,
+ExprValueKind VK,
+CheckedConversionKind CCK) {
+  CastKind CK = (Ty.getAddressSpace() != E->getType().getAddressSpace())
+? CK_AddressSpaceConversion
+: CK_NoOp;
+  return ImpCastExprToType(E, Ty, CK, VK, /*BasePath=*/nullptr, CCK);
+}
+
+ExprResult InitializationSequence::Perform(Sema &S,
+   const InitializedEntity &Entity,
+   const InitializationKind &Kind,
+   MultiExprArg Args,
+   QualType *ResultType) {
   if (Failed()) {
 Diagnose(S, Entity, Kind, Args);
 return ExprError();
@@ -7603,12 +7611,11 @@
 case SK_QualificationConversionRValue: {
   // Perform a qualification conversion; these can never go wrong.
   ExprValueKind VK =
-  Step->Kind == SK_QualificationConversionLValue ?
-  VK_LValue :
-  (Step->Kind == SK_QualificationConversionXValue ?
-   VK_XValue :
-   VK_RValue);
-  CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type, CK_NoOp, VK);
+ 

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+   .get();
 

rjmccall wrote:
> Okay.  But if `ToType` *isn't* a reference type, this will never be an 
> address-space conversion.  I feel like this code could be written more 
> clearly to express what it's trying to do.
I hope it makes more sense now. Btw, it also applies to pointer type.


https://reviews.llvm.org/D53764



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

[PATCH] D54253: [OpenCL][NFC] Improve test coverage of test/Index/opencl-types.cl

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

I am a bit confused about this testing since you are running OpenCL on 
architectures that aren't supposed to support it. Can you provide more details 
on why you are doing this?


Repository:
  rC Clang

https://reviews.llvm.org/D54253



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D54258#1296706, @richardmembarth wrote:

> CUDA maps `__shared__` internally also to `__attribute__((shared))`:
>
>   #define __annotate__(a) \
>   __attribute__((a))
>   #define __location__(a) \
>   __annotate__(a)
>   ...
>   #define __shared__ \
>   __location__(shared)
>
>
> My guess is that Clang does it just the same way and only converts to 
> `LangAS::cuda_shared` for code generation in `GetGlobalVarAddressSpace`:
>  https://clang.llvm.org/doxygen/CodeGenModule_8cpp_source.html#l03305
>  In contrast, OpenCL uses keywords that are mapped directly to 
> `LangAS::opencl_local` etc.


I agree with the change itself... but it's quite annoying that such things 
can't be tested. :(


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+   .get();
 

Anastasia wrote:
> rjmccall wrote:
> > Okay.  But if `ToType` *isn't* a reference type, this will never be an 
> > address-space conversion.  I feel like this code could be written more 
> > clearly to express what it's trying to do.
> I hope it makes more sense now. Btw, it also applies to pointer type.
The logic is wrong for pointer types; if you're converting pointers, you need 
to be checking the address space of the pointee type of the from type.

It sounds like this is totally inadequately tested; please flesh out the test 
with all of these cases.  While you're at it, please ensure that there are 
tests verifying that we don't allowing address-space changes in nested 
positions.


https://reviews.llvm.org/D53764



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

r346770 - [AST][NFC] Pack DeclRefExpr

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Tue Nov 13 09:56:44 2018
New Revision: 346770

URL: http://llvm.org/viewvc/llvm-project?rev=346770&view=rev
Log:
[AST][NFC] Pack DeclRefExpr

Move the SourceLocation to the bit-fields of Stmt + clang-format.
This saves one pointer per DeclRefExpr but otherwise NFC.


Modified:
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/include/clang/AST/Stmt.h
cfe/trunk/lib/AST/Expr.cpp

Modified: cfe/trunk/include/clang/AST/Expr.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=346770&r1=346769&r2=346770&view=diff
==
--- cfe/trunk/include/clang/AST/Expr.h (original)
+++ cfe/trunk/include/clang/AST/Expr.h Tue Nov 13 09:56:44 2018
@@ -1038,61 +1038,60 @@ class DeclRefExpr final
   private llvm::TrailingObjects {
+  friend class ASTStmtReader;
+  friend class ASTStmtWriter;
+  friend TrailingObjects;
+
   /// The declaration that we are referencing.
   ValueDecl *D;
 
-  /// The location of the declaration name itself.
-  SourceLocation Loc;
-
   /// Provides source/type location info for the declaration name
   /// embedded in D.
   DeclarationNameLoc DNLoc;
 
   size_t numTrailingObjects(OverloadToken) const {
-return hasQualifier() ? 1 : 0;
+return hasQualifier();
   }
 
   size_t numTrailingObjects(OverloadToken) const {
-return hasFoundDecl() ? 1 : 0;
+return hasFoundDecl();
   }
 
   size_t numTrailingObjects(OverloadToken) const {
-return hasTemplateKWAndArgsInfo() ? 1 : 0;
+return hasTemplateKWAndArgsInfo();
   }
 
   /// Test whether there is a distinct FoundDecl attached to the end of
   /// this DRE.
   bool hasFoundDecl() const { return DeclRefExprBits.HasFoundDecl; }
 
-  DeclRefExpr(const ASTContext &Ctx,
-  NestedNameSpecifierLoc QualifierLoc,
-  SourceLocation TemplateKWLoc,
-  ValueDecl *D, bool RefersToEnlosingVariableOrCapture,
-  const DeclarationNameInfo &NameInfo,
-  NamedDecl *FoundD,
-  const TemplateArgumentListInfo *TemplateArgs,
-  QualType T, ExprValueKind VK);
+  DeclRefExpr(const ASTContext &Ctx, NestedNameSpecifierLoc QualifierLoc,
+  SourceLocation TemplateKWLoc, ValueDecl *D,
+  bool RefersToEnlosingVariableOrCapture,
+  const DeclarationNameInfo &NameInfo, NamedDecl *FoundD,
+  const TemplateArgumentListInfo *TemplateArgs, QualType T,
+  ExprValueKind VK);
 
   /// Construct an empty declaration reference expression.
-  explicit DeclRefExpr(EmptyShell Empty)
-: Expr(DeclRefExprClass, Empty) { }
+  explicit DeclRefExpr(EmptyShell Empty) : Expr(DeclRefExprClass, Empty) {}
 
   /// Computes the type- and value-dependence flags for this
   /// declaration reference expression.
-  void computeDependence(const ASTContext &C);
+  void computeDependence(const ASTContext &Ctx);
 
 public:
   DeclRefExpr(ValueDecl *D, bool RefersToEnclosingVariableOrCapture, QualType 
T,
   ExprValueKind VK, SourceLocation L,
   const DeclarationNameLoc &LocInfo = DeclarationNameLoc())
-: Expr(DeclRefExprClass, T, VK, OK_Ordinary, false, false, false, false),
-  D(D), Loc(L), DNLoc(LocInfo) {
-DeclRefExprBits.HasQualifier = 0;
-DeclRefExprBits.HasTemplateKWAndArgsInfo = 0;
-DeclRefExprBits.HasFoundDecl = 0;
-DeclRefExprBits.HadMultipleCandidates = 0;
+  : Expr(DeclRefExprClass, T, VK, OK_Ordinary, false, false, false, false),
+D(D), DNLoc(LocInfo) {
+DeclRefExprBits.HasQualifier = false;
+DeclRefExprBits.HasTemplateKWAndArgsInfo = false;
+DeclRefExprBits.HasFoundDecl = false;
+DeclRefExprBits.HadMultipleCandidates = false;
 DeclRefExprBits.RefersToEnclosingVariableOrCapture =
 RefersToEnclosingVariableOrCapture;
+DeclRefExprBits.Loc = L;
 computeDependence(D->getASTContext());
   }
 
@@ -1112,8 +,7 @@ public:
  const TemplateArgumentListInfo *TemplateArgs = nullptr);
 
   /// Construct an empty declaration reference expression.
-  static DeclRefExpr *CreateEmpty(const ASTContext &Context,
-  bool HasQualifier,
+  static DeclRefExpr *CreateEmpty(const ASTContext &Context, bool HasQualifier,
   bool HasFoundDecl,
   bool HasTemplateKWAndArgsInfo,
   unsigned NumTemplateArgs);
@@ -1123,11 +1121,11 @@ public:
   void setDecl(ValueDecl *NewD) { D = NewD; }
 
   DeclarationNameInfo getNameInfo() const {
-return DeclarationNameInfo(getDecl()->getDeclName(), Loc, DNLoc);
+return DeclarationNameInfo(getDecl()->getDeclName(), getLocation(), DNLoc);
   }
 
-  SourceLocation getLocation() const { return Loc; }
-  void setLocation(SourceLocation L) { Loc = L; }
+  SourceLocation getLocation() const { return DeclRefExprBits.Loc; }
+  void setLocation(SourceLocation L) { Decl

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+if (StringRef(A->getValue()) == "single")
+  return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
 SmallString<128> T(FinalOutput->getValue());

grimar wrote:
> grimar wrote:
> > dblaikie wrote:
> > > If this function now takes the output file name - could it be simplified 
> > > to just always use that, rather than the conditional code that follows 
> > > and tests whether -o is specified and builds up something like the output 
> > > file name & uses the dwo suffix?
> > I am investigating this.
> So what I see now is that when the function becomes:
> 
> ```
> const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
>   const InputInfo &Output) {
>   SmallString<128> T(Output.getFilename());
>   llvm::sys::path::replace_extension(T, "dwo");
>   return Args.MakeArgString(T);
> }
> ```
> 
> Then no clang tests (check-clang) fail. This does not seem normal to me.
> I would expect that such a change should break some `-fdebug-compilation-dir` 
> relative 
> logic at least and I suspect we just have no test(s) atm.
> 
> What about if I add test case(s) and post a follow-up refactor patch for this 
> place?
> (I started work on it, but it will take some time).
Sounds good - thanks!



Comment at: test/CodeGen/split-debug-single-file.c:12
+//  RUN:   -enable-split-dwarf=split -split-dwarf-file %t.o -emit-obj -o %t.o 
%s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck 
--check-prefix=MODE-SPLIT %s
+//  MODE-SPLIT-NOT: .dwo

grimar wrote:
> dblaikie wrote:
> > This looks like an end-to-end test, which we don't usually do in Clang (or 
> > in the LLVM project in general).
> > 
> > For example, the previous testing for split-dwarf had a driver test (that 
> > tested only that the driver produced the right cc1 invocation) and a 
> > CodeGen test (that tested that the right IR was produced), but I don't see 
> > any test like this (that tested the resulting object file)?
> > 
> > I know there's a narrow gap in IR testing - things that don't go in the IR, 
> > but instead go through MCOptions  & that the SplitDwarfFile is one of those.
> > 
> > So, yeah, in this case it's a bit of a test hole - if you're going to keep 
> > this test, perhaps it could test assembly, rather than the object file? 
> > Since it doesn't need complex parsing, etc, perhaps?
> > So, yeah, in this case it's a bit of a test hole - if you're going to keep 
> > this test, perhaps it could test assembly, rather than the object file? 
> > Since it doesn't need complex parsing, etc, perhaps?
> 
> I am not sure assembly can help here. For example
> `clang main.c -S -g -gsplit-dwarf` would create single asm file output anyways
> and what I tried to check here is that we can either place .dwo sections into 
> the
> same output or into a different one depending on new option.
> 
> > For example, the previous testing for split-dwarf had a driver test (that 
> > tested only that the driver produced the right cc1 invocation) and a 
> > CodeGen test (that tested that the right IR was produced), but I don't see 
> > any test like this (that tested the resulting object file)?
> 
> I think `split-debug-filename.c` do that?
> See: 
> https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/split-debug-filename.c#L18
> 
> Also, `relax.c`: 
> https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/relax.c
> And `mips-inline-asm-abi.c`:  
> https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/mips-inline-asm-abi.c
> 
> Seems it is not common, but still acceptable?
Ah, I see/good point, split-debug-filename.c tests both the IR & then also 
tests the object headers.

Sounds good


https://reviews.llvm.org/D52296



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[Arthur O'Dwyer via Phabricator via cfe-commits]

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



Comment at: docs/LanguageExtensions.rst:1096
+  equivalent to copying the underlying bytes and then dropping the source 
object
+  on the floor.
 * ``__is_destructible`` (MSVC 2013)

@rjmccall wrote:
> trivial_abi permits annotated types to be passed and returned in registers, 
> which is ABI-breaking. Skimming the blog post, it looks like 
> trivially_relocatable does not permit this — it merely signifies that 
> destruction is a no-op after a move construction or assignment.

Not necessarily a "no-op"; my canonical example is a CopyOnlyCXX03SharedPtr 
which increments a refcount on construction and decrements on destruction. But 
move-construction plus destruction should "balance out" and result in no 
observable side effects.

> This is usefully different in the design space, since it means you can safely 
> add the attribute retroactively to e.g. std::unique_ptr, and other templates 
> can then detect that std::unique_ptr is trivially-relocatable and optimize 
> themselves to use memcpy or realloc or whatever it is that they want to do. 
> So in that sense trivial_abi is a *stronger* attribute, not a *weaker* one: 
> the property it determines ought to imply trivially_relocatable.

`trivial_abi` is an "orthogonal" attribute: you can have `trivial_abi` types 
with non-trivial constructors and destructors, which can have observable side 
effects. For example,
```
struct [[clang::trivial_abi]] DestructionAnnouncer {
~DestructionAnnouncer() { puts("hello!"); }
};
```
is `trivial_abi` (because of the annotation) yet not trivially relocatable, 
because its "move plus destroy" operation has observable side effects.

> The only interesting question in the language design that I know of is what 
> happens if you put the attribute on a template that's instantiated to contain 
> a sub-object that is definitely not trivially relocatable / trivial-ABI. For 
> trivial_abi, we decided that the attribute is simply ignored — it implicitly 
> only applies to specializations where the attribute would be legal. I haven't 
> dug into the design enough to know what trivially_relocatable decides in this 
> situation, but the three basic options are:
>
> - the attribute always has effect and allows trivial relocation regardless of 
> the subobject types; this is obviously unsafe, so it limits the safe 
> applicability of the attribute to templates
> - the attribute is ignored, like trivial_abi is
> - the attribute is ill-formed, and you'll need to add a 
> [[trivially_relocatable(bool)]] version to support templates

What happens is basically the first thing you said, except that I disagree that 
it's "obviously unsafe." Right now, conditionally trivial relocation is 
possible via template metaprogramming; see the libcxx patch at e.g.
https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
Since the attribute is an opt-in mechanism, it makes perfect sense to me that 
if you put it on a class (or class template), then it applies to the class, 
without any further sanity-checking by the compiler. The compiler has no reason 
to second-guess the programmer here.

However, there's one more interesting case. Suppose the programmer puts the 
attribute on a class that isn't relocatable at all! (For example, the union 
case @erichkeane mentioned, or a class type with a deleted destructor.) In that 
case, this patch *does* give an error... *unless* the class was produced by 
instantiating a template, in which case we *don't* give an error, because it's 
not the template-writer's fault.
https://p1144.godbolt.org/z/wSZPba


Repository:
  rC Clang

https://reviews.llvm.org/D50119



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

[PATCH] D53934: [clangd] Improve code completion for ObjC methods

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

Since I don't have commit access, @sammccall will land this.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53934



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

[PATCH] D51762: First part of the calendar stuff

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Whoops right sry again!


https://reviews.llvm.org/D51762



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

[PATCH] D54404: Exclude matchers which can have multiple results

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added a comment.

In https://reviews.llvm.org/D54404#1296224, @aaron.ballman wrote:

> In https://reviews.llvm.org/D54404#1295426, @steveire wrote:
>
> > I acknowledge and share the future-proofing concern.
> >
> > We could possibly use something trait-based instead and put the trait 
> > beside the matcher definition in ASTMatchers.h, but that doesn't really 
> > solve the problem. It only moves the problem.
>
>
> If we could somehow incorporate it into the matcher definition itself, 
> though, it means we don't have two lists of things to keep updated. You're 
> right that it doesn't eliminate the problem -- we still have to know to ask 
> the question when reviewing new matchers (so perhaps something that requires 
> you to explicitly opt-in/out would be better).
>
> Adding @sbenza in case he has ideas.


We could put something in `MatcherDescriptor` to indicate this. However, I am 
still not sure what property are we looking for here.




Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:606
+  // be used with particular arguments.
+  static std::vector ExcludedMatchers{
+  "allOf",

I don't think we are allowed to have non-trivial static storage duration 
objects like this.
You have to use llvm::ManagedStatic. Or just make it a raw array.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:624
+  "hasAnyDeclaration",
+  "hasAnyName",
+  "hasAnyParameter",

I'm not sure what goes in this list.
`hasAnyName` is here but not `hasName`.
What is ambiguous about `hasAnyName`?


Repository:
  rC Clang

https://reviews.llvm.org/D54404



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

[PATCH] D51762: First part of the calendar stuff

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ugh i cannot close it :/


https://reviews.llvm.org/D51762



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

[PATCH] D54356: Convert CheckICE into a statment visitor

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Ping?


Repository:
  rC Clang

https://reviews.llvm.org/D54356



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

[PATCH] D54355: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Ping?


Repository:
  rC Clang

https://reviews.llvm.org/D54355



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

[PATCH] D54245: [VFS] Implement `RedirectingFileSystem::getRealPath`.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Ping.


https://reviews.llvm.org/D54245



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: docs/LanguageExtensions.rst:1096
+  equivalent to copying the underlying bytes and then dropping the source 
object
+  on the floor.
 * ``__is_destructible`` (MSVC 2013)

Quuxplusone wrote:
> @rjmccall wrote:
> > trivial_abi permits annotated types to be passed and returned in registers, 
> > which is ABI-breaking. Skimming the blog post, it looks like 
> > trivially_relocatable does not permit this — it merely signifies that 
> > destruction is a no-op after a move construction or assignment.
> 
> Not necessarily a "no-op"; my canonical example is a CopyOnlyCXX03SharedPtr 
> which increments a refcount on construction and decrements on destruction. 
> But move-construction plus destruction should "balance out" and result in no 
> observable side effects.
> 
> > This is usefully different in the design space, since it means you can 
> > safely add the attribute retroactively to e.g. std::unique_ptr, and other 
> > templates can then detect that std::unique_ptr is trivially-relocatable and 
> > optimize themselves to use memcpy or realloc or whatever it is that they 
> > want to do. So in that sense trivial_abi is a *stronger* attribute, not a 
> > *weaker* one: the property it determines ought to imply 
> > trivially_relocatable.
> 
> `trivial_abi` is an "orthogonal" attribute: you can have `trivial_abi` types 
> with non-trivial constructors and destructors, which can have observable side 
> effects. For example,
> ```
> struct [[clang::trivial_abi]] DestructionAnnouncer {
> ~DestructionAnnouncer() { puts("hello!"); }
> };
> ```
> is `trivial_abi` (because of the annotation) yet not trivially relocatable, 
> because its "move plus destroy" operation has observable side effects.
> 
> > The only interesting question in the language design that I know of is what 
> > happens if you put the attribute on a template that's instantiated to 
> > contain a sub-object that is definitely not trivially relocatable / 
> > trivial-ABI. For trivial_abi, we decided that the attribute is simply 
> > ignored — it implicitly only applies to specializations where the attribute 
> > would be legal. I haven't dug into the design enough to know what 
> > trivially_relocatable decides in this situation, but the three basic 
> > options are:
> >
> > - the attribute always has effect and allows trivial relocation regardless 
> > of the subobject types; this is obviously unsafe, so it limits the safe 
> > applicability of the attribute to templates
> > - the attribute is ignored, like trivial_abi is
> > - the attribute is ill-formed, and you'll need to add a 
> > [[trivially_relocatable(bool)]] version to support templates
> 
> What happens is basically the first thing you said, except that I disagree 
> that it's "obviously unsafe." Right now, conditionally trivial relocation is 
> possible via template metaprogramming; see the libcxx patch at e.g.
> https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
> Since the attribute is an opt-in mechanism, it makes perfect sense to me that 
> if you put it on a class (or class template), then it applies to the class, 
> without any further sanity-checking by the compiler. The compiler has no 
> reason to second-guess the programmer here.
> 
> However, there's one more interesting case. Suppose the programmer puts the 
> attribute on a class that isn't relocatable at all! (For example, the union 
> case @erichkeane mentioned, or a class type with a deleted destructor.) In 
> that case, this patch *does* give an error... *unless* the class was produced 
> by instantiating a template, in which case we *don't* give an error, because 
> it's not the template-writer's fault.
> https://p1144.godbolt.org/z/wSZPba
> trivial_abi is an "orthogonal" attribute: you can have trivial_abi types with 
> non-trivial constructors and destructors, which can have observable side 
> effects. 

Let me cut this conversation short.  `trivial_abi` is not such an old and 
widely-established attribute that we are unable to revise its definition.  I am 
comfortable making the same semantic guarantees for `trivial_abi` that you're 
making for `trivially_relocatable`, because I think it is in the language's 
interest for `trivial_abi` to be strictly stronger than `trivially_relocatable`.

> What happens is basically the first thing you said, except that I disagree 
> that it's "obviously unsafe." 

Under your semantics, the attribute is an unchecked assertion about all of a 
class's subobjects.  A class template which fails to correctly apply the 
template metaprogramming trick to all of its dependently-typed subobjects — 
which can be quite awkward because it creates an extra dimension of partial 
specialization, and which breaks ABI by adding extra template parameters — will 
be silently miscompiled to allow objects to be memcpy'ed when they're 
potentially not legal to memcpy.  That is a footg

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: docs/LanguageExtensions.rst:1096
+  equivalent to copying the underlying bytes and then dropping the source 
object
+  on the floor.
 * ``__is_destructible`` (MSVC 2013)

rjmccall wrote:
> Quuxplusone wrote:
> > @rjmccall wrote:
> > > trivial_abi permits annotated types to be passed and returned in 
> > > registers, which is ABI-breaking. Skimming the blog post, it looks like 
> > > trivially_relocatable does not permit this — it merely signifies that 
> > > destruction is a no-op after a move construction or assignment.
> > 
> > Not necessarily a "no-op"; my canonical example is a CopyOnlyCXX03SharedPtr 
> > which increments a refcount on construction and decrements on destruction. 
> > But move-construction plus destruction should "balance out" and result in 
> > no observable side effects.
> > 
> > > This is usefully different in the design space, since it means you can 
> > > safely add the attribute retroactively to e.g. std::unique_ptr, and other 
> > > templates can then detect that std::unique_ptr is trivially-relocatable 
> > > and optimize themselves to use memcpy or realloc or whatever it is that 
> > > they want to do. So in that sense trivial_abi is a *stronger* attribute, 
> > > not a *weaker* one: the property it determines ought to imply 
> > > trivially_relocatable.
> > 
> > `trivial_abi` is an "orthogonal" attribute: you can have `trivial_abi` 
> > types with non-trivial constructors and destructors, which can have 
> > observable side effects. For example,
> > ```
> > struct [[clang::trivial_abi]] DestructionAnnouncer {
> > ~DestructionAnnouncer() { puts("hello!"); }
> > };
> > ```
> > is `trivial_abi` (because of the annotation) yet not trivially relocatable, 
> > because its "move plus destroy" operation has observable side effects.
> > 
> > > The only interesting question in the language design that I know of is 
> > > what happens if you put the attribute on a template that's instantiated 
> > > to contain a sub-object that is definitely not trivially relocatable / 
> > > trivial-ABI. For trivial_abi, we decided that the attribute is simply 
> > > ignored — it implicitly only applies to specializations where the 
> > > attribute would be legal. I haven't dug into the design enough to know 
> > > what trivially_relocatable decides in this situation, but the three basic 
> > > options are:
> > >
> > > - the attribute always has effect and allows trivial relocation 
> > > regardless of the subobject types; this is obviously unsafe, so it limits 
> > > the safe applicability of the attribute to templates
> > > - the attribute is ignored, like trivial_abi is
> > > - the attribute is ill-formed, and you'll need to add a 
> > > [[trivially_relocatable(bool)]] version to support templates
> > 
> > What happens is basically the first thing you said, except that I disagree 
> > that it's "obviously unsafe." Right now, conditionally trivial relocation 
> > is possible via template metaprogramming; see the libcxx patch at e.g.
> > https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
> > Since the attribute is an opt-in mechanism, it makes perfect sense to me 
> > that if you put it on a class (or class template), then it applies to the 
> > class, without any further sanity-checking by the compiler. The compiler 
> > has no reason to second-guess the programmer here.
> > 
> > However, there's one more interesting case. Suppose the programmer puts the 
> > attribute on a class that isn't relocatable at all! (For example, the union 
> > case @erichkeane mentioned, or a class type with a deleted destructor.) In 
> > that case, this patch *does* give an error... *unless* the class was 
> > produced by instantiating a template, in which case we *don't* give an 
> > error, because it's not the template-writer's fault.
> > https://p1144.godbolt.org/z/wSZPba
> > trivial_abi is an "orthogonal" attribute: you can have trivial_abi types 
> > with non-trivial constructors and destructors, which can have observable 
> > side effects. 
> 
> Let me cut this conversation short.  `trivial_abi` is not such an old and 
> widely-established attribute that we are unable to revise its definition.  I 
> am comfortable making the same semantic guarantees for `trivial_abi` that 
> you're making for `trivially_relocatable`, because I think it is in the 
> language's interest for `trivial_abi` to be strictly stronger than 
> `trivially_relocatable`.
> 
> > What happens is basically the first thing you said, except that I disagree 
> > that it's "obviously unsafe." 
> 
> Under your semantics, the attribute is an unchecked assertion about all of a 
> class's subobjects.  A class template which fails to correctly apply the 
> template metaprogramming trick to all of its dependently-typed subobjects — 
> which can be quite awkward because it creates an extra dimension of partial 
> specialization, a

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: lib/AST/Type.cpp:2234
+bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
+  QualType T = Context.getBaseElementType(*this);
+  if (T->isIncompleteType())

Quuxplusone wrote:
> erichkeane wrote:
> > Quuxplusone wrote:
> > > erichkeane wrote:
> > > > You likely want to canonicalize here.
> > > You mean `QualType T = Context.getBaseElementType(getCanonicalType());`?
> > > I can do that. For my own edification (and/or a test case), in what way 
> > > does the current code fail?
> > More like: QualType T = 
> > Context.getBaseElementType(*this).getCanonicalType();
> > 
> > This desugars typedefs in some cases, which could make the below fail.  
> > 
> > Something like this: https://godbolt.org/z/-C-Onh
> > 
> > It MIGHT still pass here, but something else might canonicalize this along 
> > the way.
> > 
> > ADDITIONALLY, I wonder if this properly handles references?  I don't think 
> > getBaseElementType will de-reference. You might want to do that as well.
> > ADDITIONALLY, I wonder if this properly handles references? I don't think 
> > getBaseElementType will de-reference. You might want to do that as well.
> 
> I actually don't want references to be stripped here: `int&` should come out 
> as "not trivially relocatable" because it is not an object type.
> https://p1144.godbolt.org/z/TbSsOA
Ah, I see.  For some reason I had it in my mind that you wanted to follow 
references.


Repository:
  rC Clang

https://reviews.llvm.org/D50119



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

r346781 - [AST][NFC] Style fixes for UnaryOperator

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Tue Nov 13 11:27:39 2018
New Revision: 346781

URL: http://llvm.org/viewvc/llvm-project?rev=346781&view=rev
Log:
[AST][NFC] Style fixes for UnaryOperator

In preparation for the patch which will move some data to the bit-fields
of Stmt. In particular, rename the private variable "Val" -> "Operand"
since the substatement is the operand of the unary operator.
Run clang-format on UnaryOperator. NFC otherwise.


Modified:
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/lib/AST/Expr.cpp

Modified: cfe/trunk/include/clang/AST/Expr.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=346781&r1=346780&r2=346781&view=diff
==
--- cfe/trunk/include/clang/AST/Expr.h (original)
+++ cfe/trunk/include/clang/AST/Expr.h Tue Nov 13 11:27:39 2018
@@ -1876,27 +1876,27 @@ private:
   unsigned Opc : 5;
   unsigned CanOverflow : 1;
   SourceLocation Loc;
-  Stmt *Val;
+  Stmt *Operand;
+
 public:
-  UnaryOperator(Expr *input, Opcode opc, QualType type, ExprValueKind VK,
-ExprObjectKind OK, SourceLocation l, bool CanOverflow)
-  : Expr(UnaryOperatorClass, type, VK, OK,
- input->isTypeDependent() || type->isDependentType(),
- input->isValueDependent(),
- (input->isInstantiationDependent() ||
-  type->isInstantiationDependentType()),
- input->containsUnexpandedParameterPack()),
-Opc(opc), CanOverflow(CanOverflow), Loc(l), Val(input) {}
+  UnaryOperator(Expr *Operand, Opcode Opc, QualType Ty, ExprValueKind VK,
+ExprObjectKind OK, SourceLocation Loc, bool CanOverflow)
+  : Expr(UnaryOperatorClass, Ty, VK, OK,
+ Operand->isTypeDependent() || Ty->isDependentType(),
+ Operand->isValueDependent(),
+ (Operand->isInstantiationDependent() ||
+  Ty->isInstantiationDependentType()),
+ Operand->containsUnexpandedParameterPack()),
+Opc(Opc), CanOverflow(CanOverflow), Loc(Loc), Operand(Operand) {}
 
   /// Build an empty unary operator.
-  explicit UnaryOperator(EmptyShell Empty)
-: Expr(UnaryOperatorClass, Empty), Opc(UO_AddrOf) { }
+  explicit UnaryOperator(EmptyShell Empty) : Expr(UnaryOperatorClass, Empty) {}
 
   Opcode getOpcode() const { return static_cast(Opc); }
   void setOpcode(Opcode O) { Opc = O; }
 
-  Expr *getSubExpr() const { return cast(Val); }
-  void setSubExpr(Expr *E) { Val = E; }
+  Expr *getSubExpr() const { return cast(Operand); }
+  void setSubExpr(Expr *E) { Operand = E; }
 
   /// getOperatorLoc - Return the location of the operator.
   SourceLocation getOperatorLoc() const { return Loc; }
@@ -1912,45 +1912,41 @@ public:
   void setCanOverflow(bool C) { CanOverflow = C; }
 
   /// isPostfix - Return true if this is a postfix operation, like x++.
-  static bool isPostfix(Opcode Op) {
-return Op == UO_PostInc || Op == UO_PostDec;
+  static bool isPostfix(Opcode Opc) {
+return Opc == UO_PostInc || Opc == UO_PostDec;
   }
 
   /// isPrefix - Return true if this is a prefix operation, like --x.
-  static bool isPrefix(Opcode Op) {
-return Op == UO_PreInc || Op == UO_PreDec;
+  static bool isPrefix(Opcode Opc) {
+return Opc == UO_PreInc || Opc == UO_PreDec;
   }
 
   bool isPrefix() const { return isPrefix(getOpcode()); }
   bool isPostfix() const { return isPostfix(getOpcode()); }
 
-  static bool isIncrementOp(Opcode Op) {
-return Op == UO_PreInc || Op == UO_PostInc;
-  }
-  bool isIncrementOp() const {
-return isIncrementOp(getOpcode());
+  static bool isIncrementOp(Opcode Opc) {
+return Opc == UO_PreInc || Opc == UO_PostInc;
   }
+  bool isIncrementOp() const { return isIncrementOp(getOpcode()); }
 
-  static bool isDecrementOp(Opcode Op) {
-return Op == UO_PreDec || Op == UO_PostDec;
-  }
-  bool isDecrementOp() const {
-return isDecrementOp(getOpcode());
+  static bool isDecrementOp(Opcode Opc) {
+return Opc == UO_PreDec || Opc == UO_PostDec;
   }
+  bool isDecrementOp() const { return isDecrementOp(getOpcode()); }
 
-  static bool isIncrementDecrementOp(Opcode Op) { return Op <= UO_PreDec; }
+  static bool isIncrementDecrementOp(Opcode Opc) { return Opc <= UO_PreDec; }
   bool isIncrementDecrementOp() const {
 return isIncrementDecrementOp(getOpcode());
   }
 
-  static bool isArithmeticOp(Opcode Op) {
-return Op >= UO_Plus && Op <= UO_LNot;
+  static bool isArithmeticOp(Opcode Opc) {
+return Opc >= UO_Plus && Opc <= UO_LNot;
   }
   bool isArithmeticOp() const { return isArithmeticOp(getOpcode()); }
 
   /// getOpcodeStr - Turn an Opcode enum value into the punctuation char it
   /// corresponds to, e.g. "sizeof" or "[pre]++"
-  static StringRef getOpcodeStr(Opcode Op);
+  static StringRef getOpcodeStr(Opcode Opc);
 
   /// Retrieve the unary opcode that corresponds to the given
   /// overloaded operator.
@@ -1961,21 +1957,21 @@ public:
   static Overloa

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: docs/LanguageExtensions.rst:1096
+  equivalent to copying the underlying bytes and then dropping the source 
object
+  on the floor.
 * ``__is_destructible`` (MSVC 2013)

Quuxplusone wrote:
> rjmccall wrote:
> > Quuxplusone wrote:
> > > @rjmccall wrote:
> > > > trivial_abi permits annotated types to be passed and returned in 
> > > > registers, which is ABI-breaking. Skimming the blog post, it looks like 
> > > > trivially_relocatable does not permit this — it merely signifies that 
> > > > destruction is a no-op after a move construction or assignment.
> > > 
> > > Not necessarily a "no-op"; my canonical example is a 
> > > CopyOnlyCXX03SharedPtr which increments a refcount on construction and 
> > > decrements on destruction. But move-construction plus destruction should 
> > > "balance out" and result in no observable side effects.
> > > 
> > > > This is usefully different in the design space, since it means you can 
> > > > safely add the attribute retroactively to e.g. std::unique_ptr, and 
> > > > other templates can then detect that std::unique_ptr is 
> > > > trivially-relocatable and optimize themselves to use memcpy or realloc 
> > > > or whatever it is that they want to do. So in that sense trivial_abi is 
> > > > a *stronger* attribute, not a *weaker* one: the property it determines 
> > > > ought to imply trivially_relocatable.
> > > 
> > > `trivial_abi` is an "orthogonal" attribute: you can have `trivial_abi` 
> > > types with non-trivial constructors and destructors, which can have 
> > > observable side effects. For example,
> > > ```
> > > struct [[clang::trivial_abi]] DestructionAnnouncer {
> > > ~DestructionAnnouncer() { puts("hello!"); }
> > > };
> > > ```
> > > is `trivial_abi` (because of the annotation) yet not trivially 
> > > relocatable, because its "move plus destroy" operation has observable 
> > > side effects.
> > > 
> > > > The only interesting question in the language design that I know of is 
> > > > what happens if you put the attribute on a template that's instantiated 
> > > > to contain a sub-object that is definitely not trivially relocatable / 
> > > > trivial-ABI. For trivial_abi, we decided that the attribute is simply 
> > > > ignored — it implicitly only applies to specializations where the 
> > > > attribute would be legal. I haven't dug into the design enough to know 
> > > > what trivially_relocatable decides in this situation, but the three 
> > > > basic options are:
> > > >
> > > > - the attribute always has effect and allows trivial relocation 
> > > > regardless of the subobject types; this is obviously unsafe, so it 
> > > > limits the safe applicability of the attribute to templates
> > > > - the attribute is ignored, like trivial_abi is
> > > > - the attribute is ill-formed, and you'll need to add a 
> > > > [[trivially_relocatable(bool)]] version to support templates
> > > 
> > > What happens is basically the first thing you said, except that I 
> > > disagree that it's "obviously unsafe." Right now, conditionally trivial 
> > > relocation is possible via template metaprogramming; see the libcxx patch 
> > > at e.g.
> > > https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
> > > Since the attribute is an opt-in mechanism, it makes perfect sense to me 
> > > that if you put it on a class (or class template), then it applies to the 
> > > class, without any further sanity-checking by the compiler. The compiler 
> > > has no reason to second-guess the programmer here.
> > > 
> > > However, there's one more interesting case. Suppose the programmer puts 
> > > the attribute on a class that isn't relocatable at all! (For example, the 
> > > union case @erichkeane mentioned, or a class type with a deleted 
> > > destructor.) In that case, this patch *does* give an error... *unless* 
> > > the class was produced by instantiating a template, in which case we 
> > > *don't* give an error, because it's not the template-writer's fault.
> > > https://p1144.godbolt.org/z/wSZPba
> > > trivial_abi is an "orthogonal" attribute: you can have trivial_abi types 
> > > with non-trivial constructors and destructors, which can have observable 
> > > side effects. 
> > 
> > Let me cut this conversation short.  `trivial_abi` is not such an old and 
> > widely-established attribute that we are unable to revise its definition.  
> > I am comfortable making the same semantic guarantees for `trivial_abi` that 
> > you're making for `trivially_relocatable`, because I think it is in the 
> > language's interest for `trivial_abi` to be strictly stronger than 
> > `trivially_relocatable`.
> > 
> > > What happens is basically the first thing you said, except that I 
> > > disagree that it's "obviously unsafe." 
> > 
> > Under your semantics, the attribute is an unchecked assertion about all of 
> > a class's subobjects.  A class template which fails to correctl

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In https://reviews.llvm.org/D54438#1296832, @Szelethus wrote:

> - Some checkers do not register themselves in all cases[1], which should 
> probably be handled at a higher level, preferably in `Checkers.td`. Warnings 
> could be emitted when a checker is explicitly enabled but will not be 
> registered. [1]




In https://reviews.llvm.org/D54438#1296832, @Szelethus wrote:

>   void ento::registerObjCDeallocChecker(CheckerManager &Mgr) {
> const LangOptions &LangOpts = Mgr.getLangOpts();
> // These checker only makes sense under MRR.
> if (LangOpts.getGC() == LangOptions::GCOnly || LangOpts.ObjCAutoRefCount)
>   return;
>  
> Mgr.registerChecker();
>   }
>


The can of worms here is as follows.

It is clear that not all checkers are applicable to all languages or all 
combinations of compiler flags. In this case, significantly different semantics 
are observed under different Objective-C memory management policies and the 
checker isn't designed to work correctly for anything but the "manual" memory 
management policy. The way the code is now written, it is impossible to 
register the checker for translation units that are compiled with incompatible 
flags.

This should not cause a warnings because it should be fine to compile different 
translation units with different flags within the same project, while the 
decision to enable the checker explicitly is done once per project.

Now, normally these decisions are handled by the Clang Driver - the gcc 
compatibility wrapper for clang that converts usual gcc-compatible run-lines 
into `-cc1` run-lines. Eg., the `unix` package is only enabled on UNIX targets 
and the `osx` package is only enabled on macOS/iOS targets and the `core` 
package is enabled on all targets and `security.insecureAPI` is disabled on PS4 
//because that's what the Driver automatically appends to the -cc1 run-line//. 
Even though `scan-build` does not call the analyzer through the Driver, it 
invokes a `-###` run-line to obtain flags first, so it still indirectly 
consults the Driver.

But the problem with the Driver is that nobody knows about this layer of 
decision-making (unlike the obvious `Checkers.td`) and therefore it's kinda 
messy to have all checkers hardcoded separately within the Driver. It kinda 
makes more sense to split the checkers into packages and let the Driver choose 
which packages are enabled.

But the problem with packages is that they are very hard to change because they 
are the UI that external GUIs rely upon. And right now we don't yet have 
separate packages for Objective-C MRR/ARC/GC modes (btw, GC needs to be 
removed).

And even if we were to go in that direction, the amount of packages would 
explode exponentially with the number of different independent flags we need to 
track. If only we had hashtags, that would have been a great direction to go.

So, generally, i suggest not to jump onto this as long as the analyzer as a 
whole works more or less correctly. Restricting functionality for the purpose 
of avoiding mistakes should be done with extreme care because, well, it 
restricts functionality. Before restricting funcitonality, we need to be more 
or less certain that nobody will ever need such functionality or that we will 
be able to provide a safe replacement when necessary without also introducing 
too much complexity (aka bugs). Which is why i recommend not to bother too much 
about it. There are much more terrible bugs all over the place, no need to 
struggle that hard to prevent these small bugs.

Another approach to the original problem would be to replace the language 
options check in registerBlahBlahChecker() with a set of language options 
checks in every checker callback. It's annoying to write and clutters the 
checker but it preserves all the functionality without messing with the 
registration process. So if you simply want to move these checks out of the way 
of your work, this is probably the way to go.


Repository:
  rC Clang

https://reviews.llvm.org/D54438



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

[PATCH] D53522: [Frontend] Include module map header declaration in dependency file output

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

The test case I've promised is

  touch a.h
  touch b.h
  touch c.h
  ln -s b.h d.h
  
  echo '#include "a.h"' > umbrella.h
  echo '#include "b.h"' >> umbrella.h
  
  echo '#include "b.h"' > test_case.c
  
  cat < module.modulemap
  module my_module {
umbrella header "umbrella.h" export *
header "c.h" export *
header "d.h" export *
  }
  EOF
  
  clang -cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=./cache 
./test_case.c -MT test_case.o -dependency-file -

Expected b.h to be present in `test_case.o` dependencies but it's not.

Keep in mind this test case can be unrelated to your change.


https://reviews.llvm.org/D53522



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

[PATCH] D54488: [AST] [analyzer] NFC: Reuse code in stable ID dumping methods.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: george.karpenkov, rsmith.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

Use the new fancy method introduced in https://reviews.llvm.org/D54486 to 
simplify some code.


Repository:
  rC Clang

https://reviews.llvm.org/D54488

Files:
  lib/AST/DeclBase.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/Stmt.cpp
  lib/StaticAnalyzer/Core/ExplodedGraph.cpp
  lib/StaticAnalyzer/Core/ProgramState.cpp


Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -70,10 +70,7 @@
 }
 
 int64_t ProgramState::getID() const {
-  Optional Out = getStateManager().Alloc.identifyObject(this);
-  assert(Out && "Wrong allocator used");
-  assert(*Out % alignof(ProgramState) == 0 && "Wrong alignment information");
-  return *Out / alignof(ProgramState);
+  return 
getStateManager().Alloc.identifyKnownAlignedObject(this);
 }
 
 ProgramStateManager::ProgramStateManager(ASTContext &Ctx,
Index: lib/StaticAnalyzer/Core/ExplodedGraph.cpp
===
--- lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -284,10 +284,7 @@
 }
 
 int64_t ExplodedNode::getID(ExplodedGraph *G) const {
-  Optional Out = G->getAllocator().identifyObject(this);
-  assert(Out && "Wrong allocator used");
-  assert(*Out % alignof(ExplodedNode) == 0 && "Wrong alignment information");
-  return *Out / alignof(ExplodedNode);
+  return G->getAllocator().identifyKnownAlignedObject(this);
 }
 
 bool ExplodedNode::isTrivial() const {
Index: lib/AST/Stmt.cpp
===
--- lib/AST/Stmt.cpp
+++ lib/AST/Stmt.cpp
@@ -303,10 +303,7 @@
 }
 
 int64_t Stmt::getID(const ASTContext &Context) const {
-  Optional Out = Context.getAllocator().identifyObject(this);
-  assert(Out && "Wrong allocator used");
-  assert(*Out % alignof(Stmt) == 0 && "Wrong alignment information");
-  return *Out / alignof(Stmt);
+  return Context.getAllocator().identifyKnownAlignedObject(this);
 }
 
 CompoundStmt::CompoundStmt(ArrayRef Stmts, SourceLocation LB,
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -2247,11 +2247,8 @@
   IsDelegating(true), IsVirtual(false), IsWritten(false), SourceOrder(0) {}
 
 int64_t CXXCtorInitializer::getID(const ASTContext &Context) const {
-  Optional Out = Context.getAllocator().identifyObject(this);
-  assert(Out && "Wrong allocator used");
-  assert(*Out % alignof(CXXCtorInitializer) == 0 &&
- "Wrong alignment information");
-  return *Out / alignof(CXXCtorInitializer);
+  return Context.getAllocator()
+.identifyKnownAlignedObject(this);
 }
 
 TypeLoc CXXCtorInitializer::getBaseClassLoc() const {
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -955,10 +955,7 @@
 static Decl::Kind getKind(const DeclContext *DC) { return DC->getDeclKind(); }
 
 int64_t Decl::getID() const {
-  Optional Out = getASTContext().getAllocator().identifyObject(this);
-  assert(Out && "Wrong allocator used");
-  assert(*Out % alignof(Decl) == 0 && "Wrong alignment information");
-  return *Out / alignof(Decl);
+  return getASTContext().getAllocator().identifyKnownAlignedObject(this);
 }
 
 const FunctionType *Decl::getFunctionType(bool BlocksToo) const {


Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -70,10 +70,7 @@
 }
 
 int64_t ProgramState::getID() const {
-  Optional Out = getStateManager().Alloc.identifyObject(this);
-  assert(Out && "Wrong allocator used");
-  assert(*Out % alignof(ProgramState) == 0 && "Wrong alignment information");
-  return *Out / alignof(ProgramState);
+  return getStateManager().Alloc.identifyKnownAlignedObject(this);
 }
 
 ProgramStateManager::ProgramStateManager(ASTContext &Ctx,
Index: lib/StaticAnalyzer/Core/ExplodedGraph.cpp
===
--- lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -284,10 +284,7 @@
 }
 
 int64_t ExplodedNode::getID(ExplodedGraph *G) const {
-  Optional Out = G->getAllocator().identifyObject(this);
-  assert(Out && "Wrong allocator used");
-  assert(*Out % alignof(ExplodedNode) == 0 && "Wrong alignment information");
-  return *Out / alignof(ExplodedNode);
+  return G->getAllocator().identifyKnownAlignedObject(this);
 }
 
 bool ExplodedNode::isTrivial() const {
Index: lib/AST/Stmt.cpp
=

[PATCH] D54489: Implement -frecord-gcc-switches

[Scott Linder via Phabricator via cfe-commits]

scott.linder created this revision.
Herald added a subscriber: cfe-commits.

See the parent revision https://reviews.llvm.org/D54487 for differences between 
this implementation and the equivalent GCC option.


Repository:
  rC Clang

https://reviews.llvm.org/D54489

Files:
  docs/ClangCommandLineReference.rst
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Driver/clang_f_opts.c

Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -538,3 +538,10 @@
 // RUN: %clang -### -S -fomit-frame-pointer -fno-omit-frame-pointer -pg %s 2>&1 | FileCheck -check-prefix=CHECK-MIX-NO-OMIT-FP-PG %s
 // CHECK-NO-MIX-OMIT-FP-PG: '-fomit-frame-pointer' not allowed with '-pg'
 // CHECK-MIX-NO-OMIT-FP-PG-NOT: '-fomit-frame-pointer' not allowed with '-pg'
+
+// RUN: %clang -### -S -frecord-gcc-switches %s 2>&1 | FileCheck -check-prefix=CHECK-RECORD-GCC-SWITCHES %s
+// RUN: %clang -### -S -fno-record-gcc-switches %s 2>&1 | FileCheck -check-prefix=CHECK-NO-RECORD-GCC-SWITCHES %s
+// RUN: %clang -### -S -fno-record-gcc-switches -frecord-gcc-switches %s 2>&1 | FileCheck -check-prefix=CHECK-RECORD-GCC-SWITCHES %s
+// RUN: %clang -### -S -frecord-gcc-switches -fno-record-gcc-switches %s 2>&1 | FileCheck -check-prefix=CHECK-NO-RECORD-GCC-SWITCHES %s
+// CHECK-RECORD-GCC-SWITCHES: "-record-command-line"
+// CHECK-NO-RECORD-GCC-SWITCHES-NOT: "-record-command-line"
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -627,6 +627,7 @@
   Args.hasFlag(OPT_ffine_grained_bitfield_accesses,
OPT_fno_fine_grained_bitfield_accesses, false);
   Opts.DwarfDebugFlags = Args.getLastArgValue(OPT_dwarf_debug_flags);
+  Opts.RecordCommandLine = Args.getLastArgValue(OPT_record_command_line);
   Opts.MergeAllConstants = Args.hasArg(OPT_fmerge_all_constants);
   Opts.NoCommon = Args.hasArg(OPT_fno_common);
   Opts.NoImplicitFloat = Args.hasArg(OPT_no_implicit_float);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4985,14 +4985,18 @@
 
   const char *Exec = D.getClangProgramPath();
 
-  // Optionally embed the -cc1 level arguments into the debug info, for build
-  // analysis.
+  // Optionally embed the -cc1 level arguments into the debug info or a
+  // section, for build analysis.
   // Also record command line arguments into the debug info if
   // -grecord-gcc-switches options is set on.
   // By default, -gno-record-gcc-switches is set on and no recording.
-  if (TC.UseDwarfDebugFlags() ||
+  auto GRecordSwitches =
   Args.hasFlag(options::OPT_grecord_gcc_switches,
-   options::OPT_gno_record_gcc_switches, false)) {
+   options::OPT_gno_record_gcc_switches, false);
+  auto FRecordSwitches =
+  Args.hasFlag(options::OPT_frecord_gcc_switches,
+   options::OPT_fno_record_gcc_switches, false);
+  if (TC.UseDwarfDebugFlags() || GRecordSwitches || FRecordSwitches) {
 ArgStringList OriginalArgs;
 for (const auto &Arg : Args)
   Arg->render(Args, OriginalArgs);
@@ -5005,8 +5009,15 @@
   Flags += " ";
   Flags += EscapedArg;
 }
-CmdArgs.push_back("-dwarf-debug-flags");
-CmdArgs.push_back(Args.MakeArgString(Flags));
+auto FlagsArgString = Args.MakeArgString(Flags);
+if (TC.UseDwarfDebugFlags() || GRecordSwitches) {
+  CmdArgs.push_back("-dwarf-debug-flags");
+  CmdArgs.push_back(FlagsArgString);
+}
+if (FRecordSwitches) {
+  CmdArgs.push_back("-record-command-line");
+  CmdArgs.push_back(FlagsArgString);
+}
   }
 
   // Host-side cuda compilation receives all device-side outputs in a single
Index: lib/CodeGen/CodeGenModule.h
===
--- lib/CodeGen/CodeGenModule.h
+++ lib/CodeGen/CodeGenModule.h
@@ -1402,6 +1402,9 @@
   /// Emit the Clang version as llvm.ident metadata.
   void EmitVersionIdentMetadata();
 
+  /// Emit the Clang commandline as llvm.commandline metadata.
+  void EmitCommandLineMetadata();
+
   /// Emits target specific Metadata for global declarations.
   void EmitTargetMetadata();
 
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -587,6 +587,9 @@
   if (getCodeGenOpts().EmitVersionIdentMetadata)
 EmitVersionIdentMetadata();
 
+  if (!getCodeGenOpts().RecordCommandLine.empty())
+EmitCommandLineMetadata();
+
   EmitTargetMeta

r346789 - DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

[David Blaikie via cfe-commits]

Author: dblaikie
Date: Tue Nov 13 12:08:13 2018
New Revision: 346789

URL: http://llvm.org/viewvc/llvm-project?rev=346789&view=rev
Log:
DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

Summary:
This saves a lot of relocations in optimized object files (at the cost
of some cost/increase in linked executable bytes), but gold's 32 bit
gdb-index support has a bug (
https://sourceware.org/bugzilla/show_bug.cgi?id=21894 ) so we can't
switch to this unconditionally. (& even if it weren't for that bug, one
might argue that some users would want to optimize in one direction or
the other - prioritizing object size or linked executable size)

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

Added:
cfe/trunk/test/CodeGen/debug-info-ranges-base-address.c
Modified:
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/include/clang/Frontend/CodeGenOptions.def
cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/Driver/debug-options.c

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=346789&r1=346788&r2=346789&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Tue Nov 13 12:08:13 2018
@@ -1780,6 +1780,10 @@ def fdebug_types_section: Flag <["-"], "
   Flags<[CC1Option]>, HelpText<"Place debug types in their own section (ELF 
Only)">;
 def fno_debug_types_section: Flag<["-"], "fno-debug-types-section">, 
Group,
   Flags<[CC1Option]>;
+def fdebug_ranges_base_address: Flag <["-"], "fdebug-ranges-base-address">, 
Group,
+  Flags<[CC1Option]>, HelpText<"Use DWARF base address selection entries in 
debug_ranges">;
+def fno_debug_ranges_base_address: Flag <["-"], 
"fno-debug-ranges-base-address">, Group,
+  Flags<[CC1Option]>;
 def fsplit_dwarf_inlining: Flag <["-"], "fsplit-dwarf-inlining">, 
Group,
   Flags<[CC1Option]>, HelpText<"Provide minimal debug info in the 
object/executable to facilitate online symbolication/stack traces in the 
absence of .dwo/.dwp files when using Split DWARF">;
 def fno_split_dwarf_inlining: Flag<["-"], "fno-split-dwarf-inlining">, 
Group,

Modified: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CodeGenOptions.def?rev=346789&r1=346788&r2=346789&view=diff
==
--- cfe/trunk/include/clang/Frontend/CodeGenOptions.def (original)
+++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def Tue Nov 13 12:08:13 2018
@@ -331,6 +331,9 @@ CODEGENOPT(PreserveVec3Type, 1, 0)
 /// Whether to emit .debug_gnu_pubnames section instead of .debug_pubnames.
 CODEGENOPT(DebugNameTable, 2, 0)
 
+/// Whether to use DWARF base address specifiers in .debug_ranges.
+CODEGENOPT(DebugRangesBaseAddress, 1, 0)
+
 CODEGENOPT(NoPLT, 1, 0)
 
 /// Whether to embed source in DWARF debug line section.

Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=346789&r1=346788&r2=346789&view=diff
==
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Tue Nov 13 12:08:13 2018
@@ -586,7 +586,8 @@ void CGDebugInfo::CreateCompileUnit() {
   CGM.getTarget().getTriple().isNVPTX()
   ? llvm::DICompileUnit::DebugNameTableKind::None
   : static_cast(
-CGOpts.DebugNameTable));
+CGOpts.DebugNameTable),
+  CGOpts.DebugRangesBaseAddress);
 }
 
 llvm::DIType *CGDebugInfo::CreateType(const BuiltinType *BT) {

Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=346789&r1=346788&r2=346789&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Tue Nov 13 12:08:13 2018
@@ -3188,6 +3188,11 @@ static void RenderDebugOptions(const Too
 ? "-gpubnames"
 : "-ggnu-pubnames");
 
+  if (Args.hasFlag(options::OPT_fdebug_ranges_base_address,
+   options::OPT_fno_debug_ranges_base_address, false)) {
+CmdArgs.push_back("-fdebug-ranges-base-address");
+  }
+
   // -gdwarf-aranges turns on the emission of the aranges section in the
   // backend.
   // Always enabled for SCE tuning.

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=346789&r1=346788&r2=346789&view=diff
==

[PATCH] D54243: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

[David Blaikie via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346789: DebugInfo: Add a driver flag for DWARF debug_ranges 
base address specifier use. (authored by dblaikie, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54243?vs=173096&id=173905#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54243

Files:
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Frontend/CodeGenOptions.def
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGen/debug-info-ranges-base-address.c
  cfe/trunk/test/Driver/debug-options.c

Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -651,6 +651,7 @@
   : Args.hasArg(OPT_gpubnames)
 ? llvm::DICompileUnit::DebugNameTableKind::Default
 : llvm::DICompileUnit::DebugNameTableKind::None);
+  Opts.DebugRangesBaseAddress = Args.hasArg(OPT_fdebug_ranges_base_address);
 
   setPGOInstrumentor(Opts, Args, Diags);
   Opts.InstrProfileOutput =
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -586,7 +586,8 @@
   CGM.getTarget().getTriple().isNVPTX()
   ? llvm::DICompileUnit::DebugNameTableKind::None
   : static_cast(
-CGOpts.DebugNameTable));
+CGOpts.DebugNameTable),
+  CGOpts.DebugRangesBaseAddress);
 }
 
 llvm::DIType *CGDebugInfo::CreateType(const BuiltinType *BT) {
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3188,6 +3188,11 @@
 ? "-gpubnames"
 : "-ggnu-pubnames");
 
+  if (Args.hasFlag(options::OPT_fdebug_ranges_base_address,
+   options::OPT_fno_debug_ranges_base_address, false)) {
+CmdArgs.push_back("-fdebug-ranges-base-address");
+  }
+
   // -gdwarf-aranges turns on the emission of the aranges section in the
   // backend.
   // Always enabled for SCE tuning.
Index: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
===
--- cfe/trunk/include/clang/Frontend/CodeGenOptions.def
+++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def
@@ -331,6 +331,9 @@
 /// Whether to emit .debug_gnu_pubnames section instead of .debug_pubnames.
 CODEGENOPT(DebugNameTable, 2, 0)
 
+/// Whether to use DWARF base address specifiers in .debug_ranges.
+CODEGENOPT(DebugRangesBaseAddress, 1, 0)
+
 CODEGENOPT(NoPLT, 1, 0)
 
 /// Whether to embed source in DWARF debug line section.
Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1780,6 +1780,10 @@
   Flags<[CC1Option]>, HelpText<"Place debug types in their own section (ELF Only)">;
 def fno_debug_types_section: Flag<["-"], "fno-debug-types-section">, Group,
   Flags<[CC1Option]>;
+def fdebug_ranges_base_address: Flag <["-"], "fdebug-ranges-base-address">, Group,
+  Flags<[CC1Option]>, HelpText<"Use DWARF base address selection entries in debug_ranges">;
+def fno_debug_ranges_base_address: Flag <["-"], "fno-debug-ranges-base-address">, Group,
+  Flags<[CC1Option]>;
 def fsplit_dwarf_inlining: Flag <["-"], "fsplit-dwarf-inlining">, Group,
   Flags<[CC1Option]>, HelpText<"Provide minimal debug info in the object/executable to facilitate online symbolication/stack traces in the absence of .dwo/.dwp files when using Split DWARF">;
 def fno_split_dwarf_inlining: Flag<["-"], "fno-split-dwarf-inlining">, Group,
Index: cfe/trunk/test/Driver/debug-options.c
===
--- cfe/trunk/test/Driver/debug-options.c
+++ cfe/trunk/test/Driver/debug-options.c
@@ -165,6 +165,10 @@
 // RUN: %clang -### -c -gsplit-dwarf %s 2>&1 | FileCheck -check-prefix=GPUB %s
 // RUN: %clang -### -c -gsplit-dwarf -gno-pubnames %s 2>&1 | FileCheck -check-prefix=NOPUB %s
 //
+// RUN: %clang -### -c -fdebug-ranges-base-address %s 2>&1 | FileCheck -check-prefix=RNGBSE %s
+// RUN: %clang -### -c %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
+// RUN: %clang -### -c -fdebug-ranges-base-address -fno-debug-ranges-base-address %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
+//
 // RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=GPUB %s
 // RUN: %clang -### -c -glld

[PATCH] D54243: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Settled on renaming the flag for consistency with, say, -fdebug-types-section, 
to -fdebug-ranges-base-address (though 'debug' is sort of ambiguous (Clang can 
produce non-DWARF debug info) but this driver (as opposed to clang-cl) is 
intended for DWARF emission, not PDB emission - and the "debug-ranges" part of 
the flag names the literal debug_ranges section (like debug-types refers to the 
debug_types section)).


Repository:
  rL LLVM

https://reviews.llvm.org/D54243



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

[PATCH] D54473: [sanitizers] Initial implementation for -fsanitize=init-locals

[Kostya Serebryany via Phabricator via cfe-commits]

kcc added a comment.

This new flag inhibits the warnings from -Wuninitialized, right? 
While this is fine for experimenting (and I want to have this in ToT to enable 
wide experimentation)
we should clearly state (in the comments) that the final intent is to make the 
feature work together with -Wuninitialized.

Another thing that we will need (and not necessary in the first change) is to 
have fine grained controls over what we zero-initialize. 
We may want to make separate decisions for pointer/non-pointer scalars, PODs, 
arrays of {scalars, pointers, PODs}, etc.


Repository:
  rC Clang

https://reviews.llvm.org/D54473



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

[PATCH] D54356: Convert CheckICE into a statment visitor

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

rsmith added a comment.

Can you explain more about the justification for this? The code today has a 
covered switch, which is useful for maintainability -- anyone adding a new 
`Expr` node gets told they need to think about and update this code. Are there 
any cases where we check for an ICE and aren't in a constant context? I would 
have expected that the fact we're asking implies that we are in a constant 
context (at least when the answer is "yes").


Repository:
  rC Clang

https://reviews.llvm.org/D54356



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

r346793 - [AST][NFC] Order the bit-field classes of Stmt like in StmtNodes.td

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Tue Nov 13 12:23:11 2018
New Revision: 346793

URL: http://llvm.org/viewvc/llvm-project?rev=346793&view=rev
Log:
[AST][NFC] Order the bit-field classes of Stmt like in StmtNodes.td

Reorder the bit-field classes and the members of the anonymous union
so that they both match the order in StmtNodes.td.

There is already a fair amount of them, and this is not going to
improve. Therefore lets try to keep some order here.

Strictly NFC.


Modified:
cfe/trunk/include/clang/AST/Stmt.h

Modified: cfe/trunk/include/clang/AST/Stmt.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Stmt.h?rev=346793&r1=346792&r2=346793&view=diff
==
--- cfe/trunk/include/clang/AST/Stmt.h (original)
+++ cfe/trunk/include/clang/AST/Stmt.h Tue Nov 13 12:23:11 2018
@@ -332,12 +332,20 @@ protected:
 SourceLocation Loc;
   };
 
-  class CharacterLiteralBitfields {
-friend class CharacterLiteral;
+  class DeclRefExprBitfields {
+friend class ASTStmtReader; // deserialization
+friend class DeclRefExpr;
 
 unsigned : NumExprBits;
 
-unsigned Kind : 3;
+unsigned HasQualifier : 1;
+unsigned HasTemplateKWAndArgsInfo : 1;
+unsigned HasFoundDecl : 1;
+unsigned HadMultipleCandidates : 1;
+unsigned RefersToEnclosingVariableOrCapture : 1;
+
+/// The location of the declaration name itself.
+SourceLocation Loc;
   };
 
   enum APFloatSemantics {
@@ -358,6 +366,14 @@ protected:
 unsigned IsExact : 1;
   };
 
+  class CharacterLiteralBitfields {
+friend class CharacterLiteral;
+
+unsigned : NumExprBits;
+
+unsigned Kind : 3;
+  };
+
   class UnaryExprOrTypeTraitExprBitfields {
 friend class UnaryExprOrTypeTraitExpr;
 
@@ -367,20 +383,12 @@ protected:
 unsigned IsType : 1; // true if operand is a type, false if an expression.
   };
 
-  class DeclRefExprBitfields {
-friend class ASTStmtReader; // deserialization
-friend class DeclRefExpr;
+  class CallExprBitfields {
+friend class CallExpr;
 
 unsigned : NumExprBits;
 
-unsigned HasQualifier : 1;
-unsigned HasTemplateKWAndArgsInfo : 1;
-unsigned HasFoundDecl : 1;
-unsigned HadMultipleCandidates : 1;
-unsigned RefersToEnclosingVariableOrCapture : 1;
-
-/// The location of the declaration name itself.
-SourceLocation Loc;
+unsigned NumPreArgs : 1;
   };
 
   class CastExprBitfields {
@@ -394,24 +402,14 @@ protected:
 unsigned BasePathIsEmpty : 1;
   };
 
-  class CallExprBitfields {
-friend class CallExpr;
-
-unsigned : NumExprBits;
-
-unsigned NumPreArgs : 1;
-  };
-
-  class ExprWithCleanupsBitfields {
-friend class ASTStmtReader; // deserialization
-friend class ExprWithCleanups;
+  class InitListExprBitfields {
+friend class InitListExpr;
 
 unsigned : NumExprBits;
 
-// When false, it must not have side effects.
-unsigned CleanupsHaveSideEffects : 1;
-
-unsigned NumObjects : 32 - 1 - NumExprBits;
+/// Whether this initializer list originally had a GNU array-range
+/// designator in it. This is a temporary marker used by CodeGen.
+unsigned HadArrayRangeDesignator : 1;
   };
 
   class PseudoObjectExprBitfields {
@@ -426,33 +424,7 @@ protected:
 unsigned ResultIndex : 32 - 8 - NumExprBits;
   };
 
-  class OpaqueValueExprBitfields {
-friend class OpaqueValueExpr;
-
-unsigned : NumExprBits;
-
-/// The OVE is a unique semantic reference to its source expressio if this
-/// bit is set to true.
-unsigned IsUnique : 1;
-  };
-
-  class ObjCIndirectCopyRestoreExprBitfields {
-friend class ObjCIndirectCopyRestoreExpr;
-
-unsigned : NumExprBits;
-
-unsigned ShouldCopy : 1;
-  };
-
-  class InitListExprBitfields {
-friend class InitListExpr;
-
-unsigned : NumExprBits;
-
-/// Whether this initializer list originally had a GNU array-range
-/// designator in it. This is a temporary marker used by CodeGen.
-unsigned HadArrayRangeDesignator : 1;
-  };
+  //===--- C++ Expression bitfields classes ---===//
 
   class TypeTraitExprBitfields {
 friend class ASTStmtReader;
@@ -472,6 +444,20 @@ protected:
 unsigned NumArgs : 32 - 8 - 1 - NumExprBits;
   };
 
+  class ExprWithCleanupsBitfields {
+friend class ASTStmtReader; // deserialization
+friend class ExprWithCleanups;
+
+unsigned : NumExprBits;
+
+// When false, it must not have side effects.
+unsigned CleanupsHaveSideEffects : 1;
+
+unsigned NumObjects : 32 - 1 - NumExprBits;
+  };
+
+  //===--- C++ Coroutines TS bitfields classes ---===//
+
   class CoawaitExprBitfields {
 friend class CoawaitExpr;
 
@@ -480,7 +466,30 @@ protected:
 unsigned IsImplicit : 1;
   };
 
+  //===--- Obj-C Expression bitfields classes ---===//
+
+  class ObjCIndirectCopyRestoreExprBitfields {
+friend class ObjCIndirectCopyRestoreExpr;
+
+unsigned : NumExprBits;
+
+unsigned ShouldCopy 

[PATCH] D54355: Use is.constant intrinsic for __builtin_constant_p

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

rsmith added inline comments.



Comment at: include/clang/AST/Expr.h:908-912
+  static ConstantExpr *Create(const ASTContext &Context, Expr *E) {
+if (ConstantExpr *CE = dyn_cast(E))
+  return CE;
+return new (Context) ConstantExpr(E);
+  }

If we're creating two `ConstantExpr` wrappers for the same expression, that 
seems like a bug in the caller to me. When does this happen?



Comment at: lib/AST/Expr.cpp:2915-2916
+  case ConstantExprClass: {
+const Expr *Exp = cast(this)->getSubExpr();
+return Exp->isConstantInitializer(Ctx, false, Culprit);
+  }

Can we just return `true` here? If not, please add a FIXME saying that we 
should be able to.



Comment at: lib/Sema/SemaExpr.cpp:4973-4974
 
+if ((ICEArguments & (1 << (ArgIx - 1))) != 0)
+  Arg = ConstantExpr::Create(Context, Arg);
+

We should create this node as part of checking that the argument is an ICE (in 
`SemaBuiltinConstantArg`).



Comment at: lib/Sema/SemaExpr.cpp:14156-14157
 
+  if (!CurContext->isFunctionOrMethod())
+E = ConstantExpr::Create(Context, E);
+

I don't understand why `CurContext` matters here. Can you explain the intent of 
this check?


Repository:
  rC Clang

https://reviews.llvm.org/D54355



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

[PATCH] D54489: Implement -frecord-gcc-switches

[Troy Johnson via Phabricator via cfe-commits]

troyj added a comment.

I realize that you're probably striving for option compatibility with gcc, but 
continuing to name it -frecord-gcc-switches when it actually records Clang 
switches seems weird to me.  It almost sounds like something that would dump 
gcc equivalents of all Clang options, or maybe let you know which Clang options 
you've used that match gcc options.  Either way, by the name -- if you aren't 
familiar with the gcc option -- it doesn't read like it records Clang options.

Would it be that bad to name it -frecord-clang-switches?  Or just 
-frecord-switches?


Repository:
  rC Clang

https://reviews.llvm.org/D54489



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

[PATCH] D54356: Convert CheckICE into a statment visitor

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

This code is called in over 90 places, so it's hard to tell if they all are in 
a constant context. Though I suppose that what this code is supposed to check 
for would work the same in a constant context as without one. I can revert this 
if you want, but to be honest the original function was terrible--it's huge and 
hard to understand what's going on. As for adding new expressions, this isn't 
the only place where a `StmtVisitor` is used. One still needs to go through all 
of those visitors to see if they need to add their expression.


Repository:
  rC Clang

https://reviews.llvm.org/D54356



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

[PATCH] D54489: Implement -frecord-gcc-switches

[Scott Linder via Phabricator via cfe-commits]

scott.linder added a comment.

In https://reviews.llvm.org/D54489#1297504, @troyj wrote:

> I realize that you're probably striving for option compatibility with gcc, 
> but continuing to name it -frecord-gcc-switches when it actually records 
> Clang switches seems weird to me.  It almost sounds like something that would 
> dump gcc equivalents of all Clang options, or maybe let you know which Clang 
> options you've used that match gcc options.  Either way, by the name -- if 
> you aren't familiar with the gcc option -- it doesn't read like it records 
> Clang options.
>
> Would it be that bad to name it -frecord-clang-switches?  Or just 
> -frecord-switches?


I agree, and this was my original plan, but then I noticed that Clang already 
implements -grecord-gcc-switches and so I decided to mirror the naming for the 
-f variant as well.

If anything I think dropping the -gcc- altogether would make the most sense. I 
don't understand why GCC includes it in the first place.


Repository:
  rC Clang

https://reviews.llvm.org/D54489



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

[PATCH] D54356: Convert CheckICE into a statment visitor

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

rsmith added a comment.

In https://reviews.llvm.org/D54356#1297470, @rsmith wrote:

> Can you explain more about the justification for this? The code today has a 
> covered switch, which is useful for maintainability -- anyone adding a new 
> `Expr` node gets told they need to think about and update this code. Are 
> there any cases where we check for an ICE and aren't in a constant context? I 
> would have expected that the fact we're asking implies that we are in a 
> constant context (at least when the answer is "yes").


Oh, I see, you want to temporarily set "IsConstantContext" to true while 
evaluating the subexpression of a `ConstantExpr`. I think that's unnecessary, 
because anywhere we ask "is this syntactically an ICE?", we always want to 
evaluate as if in a constant context (unlike places where we run the evaluator 
on an expression, where that doesn't necessarily imply anything in particular 
about the nature of the expression.)


Repository:
  rC Clang

https://reviews.llvm.org/D54356



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

[PATCH] D54356: Convert CheckICE into a statment visitor

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Okay. I'll revert this then.


Repository:
  rC Clang

https://reviews.llvm.org/D54356



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

[PATCH] D54356: Convert CheckICE into a statment visitor

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

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

In https://reviews.llvm.org/D54356#1297506, @void wrote:

> This code is called in over 90 places, so it's hard to tell if they all are 
> in a constant context. Though I suppose that what this code is supposed to 
> check for would work the same in a constant context as without one. I can 
> revert this if you want, but to be honest the original function was 
> terrible--it's huge and hard to understand what's going on. As for adding new 
> expressions, this isn't the only place where a `StmtVisitor` is used. One 
> still needs to go through all of those visitors to see if they need to add 
> their expression.


Thinking about this some more: in the case where adding a new `Stmt` node 
without considering this code is likely to result in a silent and 
initially-unnoticed bug, I think it's useful to use one of our 
covered-switch-like patterns. But I don't think this actually is such a case; 
the C ICE rules are pretty conservative in what they allow, and new `Stmt` 
nodes should nearly always be treated as non-ICE. Thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D54356



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

[PATCH] D54356: Convert CheckICE into a statment visitor

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

rsmith added a comment.

In https://reviews.llvm.org/D54356#1297522, @void wrote:

> Okay. I'll revert this then.


I don't think we necessarily need the change in the other patch that's based on 
this one, but I still think this refactoring is an improvement :)


Repository:
  rC Clang

https://reviews.llvm.org/D54356



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

[PATCH] D54356: Convert CheckICE into a statment visitor

[Bill Wendling via Phabricator via cfe-commits]

void abandoned this revision.
void added a comment.

This isn't necessary. We can assume it's in a constant context because it's 
checking for an ICE.


Repository:
  rC Clang

https://reviews.llvm.org/D54356



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

[PATCH] D54356: Convert CheckICE into a statment visitor

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In https://reviews.llvm.org/D54356#1297543, @rsmith wrote:

> In https://reviews.llvm.org/D54356#1297522, @void wrote:
>
> > Okay. I'll revert this then.
>
>
> I don't think we necessarily need the change in the other patch that's based 
> on this one, but I still think this refactoring is an improvement :)


Thanks. I can resurrect this after these changes go in. This will keep the 
resulting changes small. :-)


Repository:
  rC Clang

https://reviews.llvm.org/D54356



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