[PATCH] D43578: -ftime-report switch support in Clang

[Michael Zolotukhin via Phabricator via cfe-commits]

mzolotukhin added a comment.

Hi,

I've been monitoring compile-time for quite a while, so let my put my 2 cents 
here too.

> Who is the audience for this information?
>  What information do they want from a time report?
>  How do we present that information in a way that's not misleading (given 
> Clang's architecture)?

I would find the timers extremely useful. Even if they overlap, they still 
would 1) be a good indicator of a newly introduced problem and 2) give a rough 
idea where frontend time is spent. I agree that it wouldn't be super-accurate, 
but the numbers we usually operate are quite high (e.g.  started to 
take 1.5x time). When we decide to investigate it deeper, we can use more 
accurate tools, such as profilers. All that said, if we can make timers more 
accurate/not-overlapping, that surely would be helpful as well.

> Can we deliver useful value compared to a modern, dedicated profiling tool?

Having timers, especially producing results in the same format, as existing 
LLVM timers, would be much more convenient than using profilers. With timers I 
can simply add a compile-time flag to my test-suite run and get all the numbers 
in the end of my usual run. With profilers it's a bit more complicated.

Speaking of timers overlapping and mutual calling: LLVM timers also have this 
problem, and e.g. if there is problem is in some analysis (say, 
ScalarEvolution), we see indications in several other passes using this 
analysis (say, IndVarSimplify and LoopStrengthReduction). While it does not 
immediately show the problematic spot, it gives you pretty strong hints to 
where to look at first. So, having even not-perfect timers is still useful.

Also, I apologize for LGTMing the patch earlier while it was not properly 
reviewed - I didn't notice it didn't have cfe-commits in subscribers, and it 
had been waiting for a review for quite some time (so I assumed that all 
interested parties had their chance to look at it).

Thanks,
Michael


https://reviews.llvm.org/D43578



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

Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Nico Weber via cfe-commits]

Also, we only use auto if the type of the variable is clear. Changes like

-  for (ModuleFile  : llvm::reverse(ModuleMgr)) {
+  for (auto  : llvm::reverse(ModuleMgr)) {

are not desired.

On Sat, Apr 14, 2018, 11:09 AM Malcolm Parsons via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> On Sat, 14 Apr 2018, 14:16 Kim Gräsman,  wrote:
>
>> That would be a nice outcome of all the "run-tools-on-llvm" changes if
>> any problems were filed as bugs on the tools. We have a number of them
>> filed on iwyu, and they make for nice, concrete bugs to troubleshoot even
>> if we don't always know how to fix them.
>>
>> For this specific clang-tidy issue, do you have any ideas for how to tell
>> this loop apart from any other? I'm guessing the container is modified
>> while iterating... Or do you mean skip all non-iterator loops?
>>
>
> Non-iterator, mutable container, size checked each iteration.
>
> Clang-tidy could suggest modernisation, but not automatically fix.
>
> --
> Malcolm Parsons
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45045: [DebugInfo] Generate debug information for labels.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai updated this revision to Diff 142533.
HsiangKai added a comment.

Update test cases.


Repository:
  rC Clang

https://reviews.llvm.org/D45045

Files:
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CGStmt.cpp
  test/CodeGen/debug-label-inline.c
  test/CodeGen/debug-label.c

Index: test/CodeGen/debug-label.c
===
--- /dev/null
+++ test/CodeGen/debug-label.c
@@ -0,0 +1,16 @@
+// This test will test the correstness of generating DILabel and
+// llvm.dbg.label for labels.
+//
+// RUN: %clang_cc1 -emit-llvm %s -o - -emit-llvm -debug-info-kind=limited | FileCheck %s
+
+int f1(int a, int b) {
+  int sum;
+
+top:
+  // CHECK: call void @llvm.dbg.label(metadata [[LABEL_METADATA:!.*]]), !dbg [[LABEL_LOCATION:!.*]]
+  sum = a + b;
+  return sum;
+}
+
+// CHECK: [[LABEL_METADATA]] = !DILabel({{.*}}, name: "top", {{.*}}, line: 9)
+// CHECK: [[LABEL_LOCATION]] = !DILocation(line: 9,
Index: test/CodeGen/debug-label-inline.c
===
--- /dev/null
+++ test/CodeGen/debug-label-inline.c
@@ -0,0 +1,28 @@
+// This test will test the correctness of generating DILabel and
+// llvm.dbg.label when the label is in inlined functions.
+//
+// RUN: %clang_cc1 -O2 %s -o - -emit-llvm -debug-info-kind=limited | FileCheck %s
+inline int f1(int a, int b) {
+  int sum;
+
+top:
+  sum = a + b;
+  return sum;
+}
+
+extern int ga, gb;
+
+int f2(void) {
+  int result;
+
+  result = f1(ga, gb);
+  // CHECK: call void @llvm.dbg.label(metadata [[LABEL_METADATA:!.*]]), !dbg [[LABEL_LOCATION:!.*]]
+
+  return result;
+}
+
+// CHECK: distinct !DISubprogram(name: "f1", {{.*}}, elements: [[ELEMENTS:!.*]])
+// CHECK: [[ELEMENTS]] = !{{{.*}}, [[LABEL_METADATA]]}
+// CHECK: [[LABEL_METADATA]] = !DILabel({{.*}}, name: "top", {{.*}}, line: 8)
+// CHECK: [[INLINEDAT:!.*]] = distinct !DILocation(line: 18,
+// CHECK: [[LABEL_LOCATION]] = !DILocation(line: 8, {{.*}}, inlinedAt: [[INLINEDAT]])
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -531,6 +531,16 @@
   }
 
   EmitBlock(Dest.getBlock());
+
+  // Emit debug info for labels.
+  if (CGDebugInfo *DI = getDebugInfo()) {
+if (CGM.getCodeGenOpts().getDebugInfo() >=
+codegenoptions::LimitedDebugInfo) {
+  DI->setLocation(D->getLocation());
+  DI->EmitLabel(D, Builder);
+}
+  }
+
   incrementProfileCounter(D->getStmt());
 }
 
Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -395,6 +395,9 @@
llvm::Value *AI,
CGBuilderTy );
 
+  /// Emit call to \c llvm.dbg.label for an label.
+  void EmitLabel(const LabelDecl *D, CGBuilderTy );
+
   /// Emit call to \c llvm.dbg.declare for an imported variable
   /// declaration in a block.
   void EmitDeclareOfBlockDeclRefVariable(const VarDecl *variable,
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3640,6 +3640,33 @@
   return EmitDeclare(VD, Storage, llvm::None, Builder);
 }
 
+void CGDebugInfo::EmitLabel(const LabelDecl *D,
+CGBuilderTy ) {
+  assert(DebugKind >= codegenoptions::LimitedDebugInfo);
+  assert(!LexicalBlockStack.empty() && "Region stack mismatch, stack empty!");
+
+  if (D->hasAttr())
+return;
+
+  auto *Scope = cast(LexicalBlockStack.back());
+  llvm::DIFile *Unit = getOrCreateFile(D->getLocation());
+
+  // Get location information.
+  unsigned Line = getLineNumber(D->getLocation());
+  unsigned Column = getColumnNumber(D->getLocation());
+
+  StringRef Name = D->getName();
+
+  // Create the descriptor for the label.
+  auto *L = DBuilder.createLabel(Scope, Name, Unit, Line,
+ CGM.getLangOpts().Optimize);
+
+  // Insert an llvm.dbg.label into the current block.
+  DBuilder.insertLabel(L,
+   llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
+   Builder.GetInsertBlock());
+}
+
 llvm::DIType *CGDebugInfo::CreateSelfType(const QualType ,
   llvm::DIType *Ty) {
   llvm::DIType *CachedTy = getTypeOrNull(QualTy);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaExpr.cpp:9816
+RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+  } else {
+// C++2a [expr.spaceship]p4

rsmith wrote:
> We still need to apply the usual arithmetic conversions after converting 
> enumerations to their underlying types (eg, `<=>` on `enum E : char` converts 
> the operands first to `char` then to `int`). You could remove the `else` here 
> and make this stuff unconditional, but it's probably better to sidestep the 
> extra work and convert directly to the promoted type of the enum's underlying 
> type.
Do we still do usual arithmetic conversions if we have two enumerations of the 
same type?


https://reviews.llvm.org/D45476



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

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:9928
+// result is of type std::strong_equality
+if (CompositeTy->isFunctionPointerType() ||
+CompositeTy->isMemberPointerType() || CompositeTy->isNullPtrType())

EricWF wrote:
> rsmith wrote:
> > Please add a FIXME here to consider making the function pointer case 
> > produce strong_ordering not strong_equality, per P0946R0-Jax18 discussion / 
> > direction polls.
> Is it OK if I go ahead and implement it anyway?
Nevermind. The changes seems involved enough they should be done separately. 


https://reviews.llvm.org/D45476



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

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 4 inline comments as done.
EricWF added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:9928
+// result is of type std::strong_equality
+if (CompositeTy->isFunctionPointerType() ||
+CompositeTy->isMemberPointerType() || CompositeTy->isNullPtrType())

rsmith wrote:
> Please add a FIXME here to consider making the function pointer case produce 
> strong_ordering not strong_equality, per P0946R0-Jax18 discussion / direction 
> polls.
Is it OK if I go ahead and implement it anyway?


https://reviews.llvm.org/D45476



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

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 24 inline comments as done.
EricWF added inline comments.



Comment at: lib/AST/ExprConstant.cpp:3784
+static bool EvaluateVarDecl(EvalInfo , const VarDecl *VD,
+APValue *Dest = nullptr) {
   // We don't need to evaluate the initializer for a static local.

rsmith wrote:
> This `Dest` parameter seems to be unused, is it left behind from a previous 
> direction?
Woops. It was, thanks!



Comment at: lib/CodeGen/CGExprAgg.cpp:1000-1001
+
+  return EmitFinalDestCopy(
+  E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}

rsmith wrote:
> Hm, I wonder whether it's worthwhile to try to generate a select between the 
> comparison result values rather than their addresses. (Maybe not, since they 
> could in general be an arbitrary aggregate type, and a select on a 
> first-class aggregate value is unlikely to produce anything useful at -O0).
I was thinking something similar, but like you said, the STL implementation 
could provide an arbitrary aggregate type. 
However, I think that's a candidate for a follow up patch, so I'll add a `TODO` 
about it and leave it for now.




Comment at: lib/Sema/SemaExpr.cpp:9727-9728
+
+static bool checkNarrowingConversion(Sema , QualType ToType, Expr *E,
+ QualType FromType, SourceLocation Loc) {
+  // Check for a narrowing implicit conversion.

rsmith wrote:
> This should have a name that has something to do with three-way comparisons 
> (that is, assuming that duplicating this is the best way to customize the 
> diagnostic behavior).
I'm not sure this is the cleanest way to do it, but it seems better than trying 
to integrate it more directly with the `CheckConvertedConstantExpression` 
machinery. The semantics for `operator<=>` seems just different enough.

That being said, I'm very open to suggestions. You're the expert and resident 
compiler wizard.



Comment at: lib/Sema/SemaExpr.cpp:9807-9810
+if (!S.Context.hasSameUnqualifiedType(LHSType, RHSType)) {
+  S.InvalidOperands(Loc, LHS, RHS);
+  return QualType();
+}

rsmith wrote:
> Please implement the "straight to CWG" resolutions from 
> http://wiki.edg.com/bin/view/Wg21jacksonville2018/P0946R0-Jax18 directly 
> here. Specifically, in this case, we should allow three-way comparisons 
> between unscoped enumerations and integral types (subject to the narrowing 
> check), but not between two unrelated enumeration types, and not between a 
> scoped enumeration and an integral type.
I was thinking that if there wasn't already an issue for this behavior, there 
should be. Thanks for pointing it out.



Comment at: lib/Sema/SemaExpr.cpp:11942
 ConvertHalfVec = true;
 ResultTy = CheckCompareOperands(LHS, RHS, OpLoc, Opc, true);
+assert(ResultTy.isNull() || ResultTy->getAsCXXRecordDecl());

rsmith wrote:
> Ah, here it is, `true` is incorrectly being passed for `IsRelational` here. 
> Maybe replace that `bool` with an `enum` (or remove it entirely and have the 
> callee recompute it from `Opc`)?
Ack. Removing the parameter and re-computing it from `Opc`.


https://reviews.llvm.org/D45476



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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

I'm not sure I understand this.  The proper location for libc++ on the darwin 
layout is in the SDK, not relative to the driver.  The default behaviour is 
similar to cross-compiling, and with a (derived) SDK.  This definitely needs to 
be reviewed by @dexonsmith


Repository:
  rC Clang

https://reviews.llvm.org/D45639



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

[PATCH] D45383: Limit types of builtins that can be redeclared.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

Snipping bits from `va_defs.h`:

  #elif defined _M_ARM64
  
  void __cdecl __va_start(va_list*, ...);
  
  #define __crt_va_start_a(ap,v) ((void)(__va_start(, _ADDRESSOF(v), 
_SLOTSIZEOF(v), __alignof(v), _ADDRESSOF(v
  ...
  
  #elif defined _M_X64
  
  void __cdecl __va_start(va_list* , ...);
  
  #define __crt_va_start_a(ap, x) ((void)(__va_start(, x)))
  
  ...

This looks like a declaration to me.  Although, this is in system headers, so 
maybe we can ignore it by means of the system header suppression?  The minor 
problem with that is that clang-cl (and the clang driver with the windows 
triple) do not support `-isystem` or `-isysroot` or `--sysroot` arguments.  I 
suppose that as long as we expose the cc1 option (I imagine that clang-cl will 
pass the system paths appropriately), that is one option.


https://reviews.llvm.org/D45383



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

[PATCH] D45196: [libc++abi] Replace __sync_* functions with __libcpp_atomic_* functions.

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

I definitely like the clean up.  Not sure I understand the motivation for the 
`__libcpp_relaxed_store`, but I suppose thats because its a copy from libc++ 
where it may be more useful.


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D45196



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- [Feature] start working on handle semantic


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-handles.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-values.cpp
@@ -0,0 +1,392 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "cppcoreguidelines-const.AnalyzeValues", value: 1},\
+// RUN:   {key: "cppcoreguidelines-const.AnalyzeHandles", value: 0},\
+// RUN:   {key: "cppcoreguidelines-const.WarnPointersAsValues", value: 1}]}' \
+// RUN: --
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  function_inout_pointer(_local1);
+
+  // Prevents const.
+  int np_local2 = 42;
+  out = _local2; // This returns and invalid address, its just about the AST
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int *const p0_p_local0 = _local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_pointer(_local1);
+}
+
+void function_inout_ref(int );
+void function_in_ref(const int );
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int _np_local0 = np_local0;
+  int _np_local0 = np_local0;
+  const int _np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int _p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return _local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const
+  return _local1;
+}
+
+double _const_ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 42.42;
+  

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- [Feature] implement array and method access
- [Feature] finalize value semantic


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp

Index: test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp
@@ -0,0 +1,387 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  function_inout_pointer(_local1);
+
+  // Prevents const.
+  int np_local2 = 42;
+  out = _local2; // This returns and invalid address, its just about the AST
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int *const p0_p_local0 = _local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_pointer(_local1);
+}
+
+void function_inout_ref(int );
+void function_in_ref(const int );
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int _np_local0 = np_local0;
+  int _np_local0 = np_local0;
+  const int _np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int _p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return _local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const
+  return _local1;
+}
+
+double _const_ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 42.42;
+  return np_local0;
+}
+
+const double _ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 

[PATCH] D45319: [Atomics] warn about misaligned atomic accesses using libcalls

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

Sorry for the delay, didn't see the changes earlier.




Comment at: clang/lib/CodeGen/CGAtomic.cpp:883
   if (UseLibcall) {
+CGM.getDiags().Report(E->getLocStart(), diag::warn_atomic_op_misaligned);
+

It is kinda unfortunate that you need to look up 125 lines to get the context 
that the call here is implied by a lack of alignment.  Perhaps we can rename 
`UseLibcall` to `UnsuitableAligned` or something?


Repository:
  rC Clang

https://reviews.llvm.org/D45319



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

[clang-tools-extra] r330087 - [clangd] Fix label and snippet for funcs in the index

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Sat Apr 14 09:27:35 2018
New Revision: 330087

URL: http://llvm.org/viewvc/llvm-project?rev=330087=rev
Log:
[clangd] Fix label and snippet for funcs in the index

This is a follow-up to r330004 to fix functions with required template args,
e.g. std::make_shared.

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

Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=330087=330086=330087=diff
==
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Sat Apr 14 
09:27:35 2018
@@ -27,16 +27,11 @@ namespace clang {
 namespace clangd {
 
 namespace {
-/// If \p ND is a template specialization, returns the primary template.
+/// If \p ND is a template specialization, returns the described template.
 /// Otherwise, returns \p ND.
 const NamedDecl (const NamedDecl ) {
-  if (auto Cls = dyn_cast()) {
-if (auto T = Cls->getDescribedTemplate())
-  return *T;
-  } else if (auto Func = dyn_cast()) {
-if (auto T = Func->getPrimaryTemplate())
-  return *T;
-  }
+  if (auto T = ND.getDescribedTemplate())
+return *T;
   return ND;
 }
 

Modified: clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp?rev=330087=330086=330087=diff
==
--- clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp Sat Apr 14 
09:27:35 2018
@@ -201,8 +201,8 @@ template 
 class vector {
 };
 
-template 
-vector make_vector(Ty* begin, Ty* end) {}
+template 
+vector make_vector(Arg A) {}
 )cpp";
 
   FileIndex M;
@@ -222,9 +222,9 @@ vector make_vector(Ty* begin, Ty* en
 }
 
 if (Sym.Name == "make_vector") {
-  EXPECT_EQ(Sym.CompletionLabel, "make_vector(Ty *begin, Ty *end)");
+  EXPECT_EQ(Sym.CompletionLabel, "make_vector(Arg A)");
   EXPECT_EQ(Sym.CompletionSnippetInsertText,
-"make_vector(${1:Ty *begin}, ${2:Ty *end})");
+"make_vector<${1:class Ty}>(${2:Arg A})");
   EXPECT_EQ(Sym.CompletionPlainInsertText, "make_vector");
   SeenMakeVector = true;
 }


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

[PATCH] D43578: -ftime-report switch support in Clang

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

In https://reviews.llvm.org/D43578#1067974, @rsmith wrote:

> In https://reviews.llvm.org/D43578#1067891, @kimgr wrote:
>
> > I disagreed up until the last paragraph :)
>
>
> Can you say which things you disagree with? There seem to be two important 
> facts here:
>
> 1. LLVM's timing infrastructure cannot correctly report timings when one 
> function containing a timing region calls another. That will happen all the 
> time with this patch applied.
> 2. Clang's architecture means that if you time the amount of time spent in, 
> say, the "ParseTemplate" function, you have not computed the amount of time 
> spent parsing templates, because the parser calls into Sema, which might 
> perform tasks that are only somewhat related to parsing the template (such as 
> performing other instantiations), and likewise Sema calls into other layers 
> (such as AST file deserialization and code generation).
>
>   The first might have been fixed in LLVM at this point, but I can't see any 
> evidence of that in LLVM's Timer implementation. And the second seems even 
> more fundamental. But if there's some way around that, which would allow us 
> to produce (correct!) numbers for times spent parsing / whatever else, 
> without, for instance, adding a correctness requirement that we annotate 
> every Parser -> Sema entry point with a timer, then that's definitely 
> something we should discuss.


Ah, I see. I read your original message as dismissive as in users don't need to 
know where time is spent.

I tried to make the case that users do care where time is spent, because it can 
give a hint as to what part of their physical/logical design they can work on 
to get better compile times.

But I think you were really saying that LLVM and Clang as designed can't 
provide anything perfect, only slightly misleading information?

Sorry for the misunderstanding.

- Kim


https://reviews.llvm.org/D43578



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

[PATCH] D43576: Solution to fix PR27066 - Redefinition with same mangled name as another definition (dllexport and uuid)

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

rsmith added inline comments.



Comment at: include/clang/Sema/AttributeList.h:601
+  sizeof(AttributeList)
+  + (sizeof(DeclSpecUuidDecl) + sizeof(void *) + sizeof(ArgsUnion) - 1)
 / sizeof(void*) * sizeof(void*)

You're not storing a `DeclSpecUuidDecl` within the attribute, so you don't need 
to reserve enough space for one here. In fact, this constant appears to be 
unused, so you can just remove it.



Comment at: lib/AST/Decl.cpp:4561
+
+DeclSpecUuidDecl * DeclSpecUuidDecl::Create(const ASTContext , DeclContext 
*DC,
+SourceLocation IdLoc,

No space after `*`, please.



Comment at: lib/AST/Decl.cpp:4564
+StringRef UuidVal)
+{
+  return new (C, DC) DeclSpecUuidDecl(DeclSpecUuid, DC, IdLoc, UuidVal);

`{` on the previous line, please.



Comment at: lib/AST/DeclCXX.cpp:1727
   ((getName() == "IUnknown" &&
-Uuid->getGuid() == "---C000-0046") ||
+Uuid->getDeclSpecUuidDecl()->getStrUuid() == 
"---C000-0046") ||
(getName() == "IDispatch" &&

It would be nice to have a convenience accessor on `UuidAttr` to get the UUID 
as a string. (You can add one using `AdditionalMembers` in Attr.td.)



Comment at: lib/Parse/ParseDecl.cpp:568-583
+// Clean up the string from the "\" at begining and at end.
+StringRef UuidStr1 = UuidStr.ltrim('\"');
+StringRef TrimmedUuidStr = UuidStr1.rtrim('\"');
+DeclSpecUuidDecl *ArgDecl =
+  DeclSpecUuidDecl::Create(Actions.getASTContext(),
+   Actions.getFunctionLevelDeclContext(),
+   SourceLocation(),

The `Parser` should not create `Decl`s (that's a layering violation). There are 
two ways to handle this:

1) Add a `Sema` method to act on a UUID attribute string, and return a pointer 
that you can stash in the `Attrs` list.
2) Just store a string in the `Attrs` list, and move the code to convert the 
string into a `DeclSpecUuidDecl*` and store that on the `UuidAttr` into `Sema`.

I'd strongly prefer the second option; from the point of view of the parser, we 
can still just treat this as an attribute that takes a string, and we can do 
the string -> `DeclSpecUuidDecl` handling entirely within the semantic analysis 
for the attribute (in `Sema`).



Comment at: lib/Parse/ParseDecl.cpp:572
+DeclSpecUuidDecl *ArgDecl =
+  DeclSpecUuidDecl::Create(Actions.getASTContext(),
+   Actions.getFunctionLevelDeclContext(),

What should happen if multiple declarations (of distinct entities) use the same 
`__declspec(uuid(...))` attribute? Should you get a redefinition error for the 
attribute or should they all share the same UUID entity? Either way, we'll want 
to do some (minimal) UUID lookup and build a redeclaration chain for 
`DeclSpecUuidDecl`s.



Comment at: lib/Sema/SemaDeclAttr.cpp:4937-4938
-  return nullptr;
-Diag(UA->getLocation(), diag::err_mismatched_uuid);
-Diag(Range.getBegin(), diag::note_previous_uuid);
-D->dropAttr();

Do we still diagnose UUID mismatches somewhere else?


https://reviews.llvm.org/D43576



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

[PATCH] D43576: Solution to fix PR27066 - Redefinition with same mangled name as another definition (dllexport and uuid)

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 142516.

https://reviews.llvm.org/D43576

Files:
  include/clang/AST/Decl.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/Basic/Attr.td
  include/clang/Basic/DeclNodes.td
  include/clang/Sema/AttributeList.h
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/AST/DeclBase.cpp
  lib/AST/DeclCXX.cpp
  lib/Parse/ParseDecl.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -322,12 +322,15 @@
 OS << "\" << get" << getUpperName() << "().getAsString() << \"";
   else if (type == "ParamIdx")
 OS << "\" << get" << getUpperName() << "().getSourceIndex() << \"";
-  else
+  else if (type == "DeclSpecUuidDecl *") {
+OS << "\" << get" << getUpperName() << "() << \"";
+	} else
 OS << "\" << get" << getUpperName() << "() << \"";
 }
 
 void writeDump(raw_ostream ) const override {
-  if (type == "FunctionDecl *" || type == "NamedDecl *") {
+  if (type == "FunctionDecl *" || type == "NamedDecl *" ||
+	  (type == "DeclSpecUuidDecl *")) {
 OS << "OS << \" \";\n";
 OS << "dumpBareDeclRef(SA->get" << getUpperName() << "());\n"; 
   } else if (type == "IdentifierInfo *") {
@@ -1280,6 +1283,8 @@
 Ptr = llvm::make_unique(Arg, Attr, "ParamIdx");
   else if (ArgName == "VersionArgument")
 Ptr = llvm::make_unique(Arg, Attr);
+  else if (ArgName == "DeclSpecUuidDeclArgument")
+Ptr = llvm::make_unique(Arg, Attr, "DeclSpecUuidDecl *");
 
   if (!Ptr) {
 // Search in reverse order so that the most-derived type is handled first.
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -535,6 +535,15 @@
   return Inst;
 }
 
+Decl *
+TemplateDeclInstantiator::VisitDeclSpecUuidDecl(DeclSpecUuidDecl *D) {
+  DeclSpecUuidDecl *Inst = DeclSpecUuidDecl::Create(SemaRef.Context, Owner,
+D->getLocation(),
+D->getStrUuid());
+  Owner->addDecl(Inst);
+  return Inst;
+}
+
 Decl *TemplateDeclInstantiator::InstantiateTypedefNameDecl(TypedefNameDecl *D,
bool IsTypeAlias) {
   bool Invalid = false;
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -573,7 +573,7 @@
   return ExprError(Diag(TypeidLoc, diag::err_uuidof_without_guid));
 if (UuidAttrs.size() > 1)
   return ExprError(Diag(TypeidLoc, diag::err_uuidof_with_multiple_guids));
-UuidStr = UuidAttrs.back()->getGuid();
+UuidStr = UuidAttrs.back()->getDeclSpecUuidDecl()->getStrUuid();
   }
 
   return new (Context) CXXUuidofExpr(TypeInfoType.withConst(), Operand, UuidStr,
@@ -596,7 +596,8 @@
 return ExprError(Diag(TypeidLoc, diag::err_uuidof_without_guid));
   if (UuidAttrs.size() > 1)
 return ExprError(Diag(TypeidLoc, diag::err_uuidof_with_multiple_guids));
-  UuidStr = UuidAttrs.back()->getGuid();
+  UuidStr = UuidAttrs.back()->getDeclSpecUuidDecl()->getStrUuid();
+
 }
   }
 
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4930,14 +4930,10 @@
 //===--===//
 
 UuidAttr *Sema::mergeUuidAttr(Decl *D, SourceRange Range,
-  unsigned AttrSpellingListIndex, StringRef Uuid) {
-  if (const auto *UA = D->getAttr()) {
-if (UA->getGuid().equals_lower(Uuid))
-  return nullptr;
-Diag(UA->getLocation(), diag::err_mismatched_uuid);
-Diag(Range.getBegin(), diag::note_previous_uuid);
-D->dropAttr();
-  }
+  unsigned AttrSpellingListIndex,
+  DeclSpecUuidDecl *Uuid) {
+  if (D->getAttr())
+return nullptr;
 
   return ::new (Context) UuidAttr(Range, Context, Uuid, AttrSpellingListIndex);
 }
@@ -4949,9 +4945,9 @@
 return;
   }
 
-  StringRef StrRef;
-  SourceLocation LiteralLoc;
-  if (!S.checkStringLiteralArgumentAttr(AL, 0, StrRef, ))
+  StringRef  StrRef = AL.getUuidDecl()->getStrUuid();
+  SourceLocation LiteralLoc = AL.getLoc();
+  if (StrRef.empty())
 return;
 
   // GUID format is "----" or
@@ -4987,7 +4983,9 @@
 S.Diag(AL.getLoc(), diag::warn_atl_uuid_deprecated);
 
   UuidAttr *UA = S.mergeUuidAttr(D, AL.getRange(),
- 

[PATCH] D43578: -ftime-report switch support in Clang

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

rsmith added a comment.

In https://reviews.llvm.org/D43578#1067879, @avt77 wrote:

> In https://reviews.llvm.org/D43578#1067768, @rsmith wrote:
>
> > Last time I looked at doing this, I found that LLVM's timer infrastructure 
> > was fundamentally unsuitable for adding timers like these to Clang.
>
>
> Thank you for this answer. As I understand we should close both this review 
> and https://reviews.llvm.org/D45619


I think https://reviews.llvm.org/D45619 is a good change, and I'd like to see 
that get committed.

If LLVM's timer infrastructure can be improved to correctly track time in 
recursive and mutually-recursive timing regions (and I believe it can be), we 
could continue with this patch; if you want to pursue that, my first questions 
would be:

Who is the audience for this information?
What information do they want from a time report?
How do we present that information in a way that's not misleading (given 
Clang's architecture)?
Can we deliver useful value compared to a modern, dedicated profiling tool?

> but you'd like to see something like MS has here 
> , 
> right?

Yes, that's the kind of information I think we should be looking at adding here 
-- I think that's exactly what Clang users would be looking for when they reach 
out to a compilation time report to try to figure out why their compile is slow.


https://reviews.llvm.org/D43578



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

[PATCH] D43576: Solution to fix PR27066 - Redefinition with same mangled name as another definition (dllexport and uuid)

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

In https://reviews.llvm.org/D43576#1016295, @majnemer wrote:

> We should really, really avoid making this sort of change without first 
> trying to desugar uuidof into a reference to a variable. That would solve a 
> ton of problems, problems like this one.


Not sure I fully understand what you are proposing?

Are you proposing that generated symbols like this:
??4?$A@$E?_GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3@@3U__s_GUID@@B@@QEAAAEAV0@AEBV0@@Z
 
Be de-sugared? Wouldn't that be different that what MS is doing? 
Can you please give me more details about what you are thinking of?
Thanks.

In https://reviews.llvm.org/D43576#1019943, @rsmith wrote:

> In https://reviews.llvm.org/D43576#1019703, @zahiraam wrote:
>
> > Currently this declaration:
> > struct
> >  __declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
> >  S1;
> >
> > a CXXRecordDecl type is generated with attributes (the uuid being an 
> > attribute). Are you proposing that instead a new type is generated for S1 
> > say something like CXXUuidRecord? And create also a new TagType that 
> > corresponds to this new Decl?
>
>
> No. Concretely, I'd suggest we create a new `UuidDecl` object representing 
> the `_GUID` object. An instance of `UuidDecl` would be created and owned by 
> the `uuid` attribute, and `__uuidof(X)` would denote the `UuidDecl` owned by 
> the `uuid` attribute on the `CXXRecordDecl`. We'd also need some way to form 
> redeclaration chains for `UuidDecl`s (which means we'll need a map from UUID 
> to `UuidDecl` somewhere, perhaps on the `ASTContext`).




In https://reviews.llvm.org/D43576#1019943, @rsmith wrote:

> In https://reviews.llvm.org/D43576#1019703, @zahiraam wrote:
>
> > Currently this declaration:
> > struct
> >  __declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
> >  S1;
> >
> > a CXXRecordDecl type is generated with attributes (the uuid being an 
> > attribute). Are you proposing that instead a new type is generated for S1 
> > say something like CXXUuidRecord? And create also a new TagType that 
> > corresponds to this new Decl?
>
>
> No. Concretely, I'd suggest we create a new `UuidDecl` object representing 
> the `_GUID` object. An instance of `UuidDecl` would be created and owned by 
> the `uuid` attribute, and `__uuidof(X)` would denote the `UuidDecl` owned by 
> the `uuid` attribute on the `CXXRecordDecl`. We'd also need some way to form 
> redeclaration chains for `UuidDecl`s (which means we'll need a map from UUID 
> to `UuidDecl` somewhere, perhaps on the `ASTContext`).


As per Richard comment. I have added a UuidDecl owned by the uuid attribute. 
This patch concerns only this change. It is not a patch to fix the bug yet. I 
want to make sure that we are in agreement with my changes first. Then I will 
change the code that deal with ParseCXUuid. 
Comments are welcome. 
Thanks.


https://reviews.llvm.org/D43576



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

Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Malcolm Parsons via cfe-commits]

On Sat, 14 Apr 2018, 14:16 Kim Gräsman,  wrote:

> That would be a nice outcome of all the "run-tools-on-llvm" changes if any
> problems were filed as bugs on the tools. We have a number of them filed on
> iwyu, and they make for nice, concrete bugs to troubleshoot even if we
> don't always know how to fix them.
>
> For this specific clang-tidy issue, do you have any ideas for how to tell
> this loop apart from any other? I'm guessing the container is modified
> while iterating... Or do you mean skip all non-iterator loops?
>

Non-iterator, mutable container, size checked each iteration.

Clang-tidy could suggest modernisation, but not automatically fix.

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

[PATCH] D43578: -ftime-report switch support in Clang

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

rsmith added a comment.

In https://reviews.llvm.org/D43578#1067891, @kimgr wrote:

> I disagreed up until the last paragraph :)


Can you say which things you disagree with? There seem to be two important 
facts here:

1. LLVM's timing infrastructure cannot correctly report timings when one 
function containing a timing region calls another. That will happen all the 
time with this patch applied.
2. Clang's architecture means that if you time the amount of time spent in, 
say, the "ParseTemplate" function, you have not computed the amount of time 
spent parsing templates, because the parser calls into Sema, which might 
perform tasks that are only somewhat related to parsing the template (such as 
performing other instantiations), and likewise Sema calls into other layers 
(such as AST file deserialization and code generation).

The first might have been fixed in LLVM at this point, but I can't see any 
evidence of that in LLVM's Timer implementation. And the second seems even more 
fundamental. But if there's some way around that, which would allow us to 
produce (correct!) numbers for times spent parsing / whatever else, without, 
for instance, adding a correctness requirement that we annotate every Parser -> 
Sema entry point with a timer, then that's definitely something we should 
discuss.


https://reviews.llvm.org/D43578



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

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

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

rsmith added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:9928
+// result is of type std::strong_equality
+if (CompositeTy->isFunctionPointerType() ||
+CompositeTy->isMemberPointerType() || CompositeTy->isNullPtrType())

Please add a FIXME here to consider making the function pointer case produce 
strong_ordering not strong_equality, per P0946R0-Jax18 discussion / direction 
polls.


https://reviews.llvm.org/D45476



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

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

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

rsmith added a comment.

Thanks! This is looking pretty close.




Comment at: include/clang/AST/ComparisonCategories.h:78
+public:
+  /// \brief Wether Sema has fully checked the type and result values for this
+  ///   comparison category types before.

Typo "Wether".



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9377
+def err_implied_comparison_category_type_not_found : Error<
+  "cannot deduce return type of operator<=> because type %0 was not found; "
+  "include ">;

Nit: you have inconsistent quoting of 'operator<=>' between this diagnostic and 
the next.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9387
+  "standard library implementation of comparison category %0 is not supported; 
"
+   "failed to build reference to member '%1'">;
 } // end of sema component.

This seems a bit too implementation-detaily -- how about something vaguer like 
"[...] not supported; member '%1' does not have expected form"?

(I suppose it doesn't matter too much; only standard library implementers are 
likely to ever see this diagnostic anyway.)



Comment at: lib/AST/ComparisonCategories.cpp:51
+if (!StdNS) {
+  StdNS = NamespaceDecl::Create(
+  const_cast(Ctx), Ctx.getTranslationUnitDecl(),

We shouldn't be creating a 'namespace std' here. If there is no existing such 
namespace, our lookups should just fail.



Comment at: lib/AST/ExprConstant.cpp:3784
+static bool EvaluateVarDecl(EvalInfo , const VarDecl *VD,
+APValue *Dest = nullptr) {
   // We don't need to evaluate the initializer for a static local.

This `Dest` parameter seems to be unused, is it left behind from a previous 
direction?



Comment at: lib/AST/ExprConstant.cpp:8824
+  return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, []() -> bool {
+llvm_unreachable("operator<=> should have been evaluated to a result");
+  });

I'm not convinced this is a legitimate use of `llvm_unreachable` -- it seems to 
me that there could be all sorts of types for which Sema can form a `<=>` 
expression but that we don't handle here, such as vector types. Maybe issue an 
`FFDiag` here, or just call the base class version of this function, which I 
think should do it for you?



Comment at: lib/CodeGen/CGExprAgg.cpp:947-954
+return CGF.ErrorUnsupported(
+E, "aggregate binary expression with complex arguments");
+  if (ArgTy->isVectorType())
+return CGF.ErrorUnsupported(
+E, "aggregate binary expression with vector arguments");
+  if (!ArgTy->isIntegralOrEnumerationType() && !ArgTy->isRealFloatingType() &&
+  !ArgTy->isPointerType() && !ArgTy->isMemberPointerType())

Instead of "aggregate binary expression" in all of these errors, how about 
"three-way comparison"?



Comment at: lib/CodeGen/CGExprAgg.cpp:1000-1001
+
+  return EmitFinalDestCopy(
+  E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}

Hm, I wonder whether it's worthwhile to try to generate a select between the 
comparison result values rather than their addresses. (Maybe not, since they 
could in general be an arbitrary aggregate type, and a select on a first-class 
aggregate value is unlikely to produce anything useful at -O0).



Comment at: lib/Sema/SemaDeclCXX.cpp:8903-8916
+  // Build the initial category information
+  RecordDecl *CCDecl = nullptr;
+  // Lookup the record for the category type
+  if (auto Std = getStdNamespace()) {
+LookupResult Result(*this, ().get(Name),
+SourceLocation(), Sema::LookupTagName);
+if (LookupQualifiedName(Result, Std))

I don't think this makes sense: `CompCategories::lookupInfo` already did the 
lookup we wanted here; checking some other declaration just invites the two 
lookups being inconsistent in some way. I think you should just check whether 
`CachedInfo` is null here, and if so, produce the "type not found" error.



Comment at: lib/Sema/SemaDeclCXX.cpp:8936-8948
+  // Build each of the require values and store them in Info.
+  for (CCVT CCV : Values) {
+StringRef ValueName = ComparisonCategories::getResultString(CCV);
+QualType Ty(CCDecl->getTypeForDecl(), 0);
+DeclContext *LookupCtx = computeDeclContext(Ty);
+LookupResult Found(*this, ().get(ValueName), Loc,
+   Sema::LookupOrdinaryName);

Likewise here, you should query the `CachedInfo` object for these values and 
check them, rather than looking them up again in a different way.



Comment at: lib/Sema/SemaDeclCXX.cpp:8966
+  assert(CachedInfo->Kind == Kind);
+  CachedInfo->IsFullyChecked = true;
+  return QualType(CachedInfo->CCDecl->getTypeForDecl(), 0);

I would 

[PATCH] D35181: Defer addition of keywords to identifier table when loading AST

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from a few small nits.




Comment at: include/clang/Basic/IdentifierTable.h:471
+  /// \brief Create the identifier table.
+  IdentifierTable(IdentifierInfoLookup *externalLookup = nullptr);
+

`ExternalLookup` to match coding conventions. Also, can you mark this 
constructor as explicit? Might as well change the other constructor as well 
since it's being reformatted.


Repository:
  rC Clang

https://reviews.llvm.org/D35181



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

[PATCH] D35181: Defer addition of keywords to identifier table when loading AST

[Johann Klähn via Phabricator via cfe-commits]

jklaehn added a comment.

friendly ping? :)


Repository:
  rC Clang

https://reviews.llvm.org/D35181



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

[PATCH] D45441: [HIP] Add predefined macros __HIPCC__ and __HIP_DEVICE_COMPILE__

[John McCall via Phabricator via cfe-commits]

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

If so, LGTM.




Comment at: lib/Frontend/InitPreprocessor.cpp:473
+  Builder.defineMacro("__HIP_DEVICE_COMPILE__");
+  }
 }

I assume these names are defined by the HIP spec.


https://reviews.llvm.org/D45441



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

[PATCH] D45489: [HIP] Add input type for HIP

[John McCall via Phabricator via cfe-commits]

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

LGTM.


https://reviews.llvm.org/D45489



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

Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Kim Gräsman via cfe-commits]

That would be a nice outcome of all the "run-tools-on-llvm" changes if any
problems were filed as bugs on the tools. We have a number of them filed on
iwyu, and they make for nice, concrete bugs to troubleshoot even if we
don't always know how to fix them.

For this specific clang-tidy issue, do you have any ideas for how to tell
this loop apart from any other? I'm guessing the container is modified
while iterating... Or do you mean skip all non-iterator loops?

- Kim

Den lör 14 apr. 2018 12:20Malcolm Parsons via cfe-commits <
cfe-commits@lists.llvm.org> skrev:

> On Sat, 14 Apr 2018, 04:22 Richard Trieu via cfe-commits, <
> cfe-commits@lists.llvm.org> wrote:
>
>> I was tracking down a similar issue to the lldb issue before noticing the
>> change was reverted.  The bad change that lead to it is:
>>
>>  // Load pending declaration chains.
>> -for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
>> -  loadPendingDeclChain(PendingDeclChains[I].first,
>> PendingDeclChains[I].second);
>> +for (const auto  : PendingDeclChains)
>> +  loadPendingDeclChain(I.first, I.second);
>>  PendingDeclChains.clear();
>>
>> Although the two looks like similar, the vector PendingDeclChains is a
>> class member and gets new elements during loop runs.  Once enough elements
>> are added to the vector, it get reallocated to a larger memory, but the
>> loop is still trying to process the old, now freed, memory.  Using an index
>> and checking the size every loop is the right way to process this vector.
>>
>
> Should clang-tidy handle this type of loop differently?
>
> --
> Malcolm Parsons
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Malcolm Parsons via cfe-commits]

On Sat, 14 Apr 2018, 04:22 Richard Trieu via cfe-commits, <
cfe-commits@lists.llvm.org> wrote:

> I was tracking down a similar issue to the lldb issue before noticing the
> change was reverted.  The bad change that lead to it is:
>
>  // Load pending declaration chains.
> -for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
> -  loadPendingDeclChain(PendingDeclChains[I].first,
> PendingDeclChains[I].second);
> +for (const auto  : PendingDeclChains)
> +  loadPendingDeclChain(I.first, I.second);
>  PendingDeclChains.clear();
>
> Although the two looks like similar, the vector PendingDeclChains is a
> class member and gets new elements during loop runs.  Once enough elements
> are added to the vector, it get reallocated to a larger memory, but the
> loop is still trying to process the old, now freed, memory.  Using an index
> and checking the size every loop is the right way to process this vector.
>

Should clang-tidy handle this type of loop differently?

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

[PATCH] D45491: [analyzer] Do not invalidate the `this` pointer.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D45491#1067852, @NoQ wrote:

> Yeah, i think this makes sense, thanks! It feels a bit weird that we have to 
> add it as an exception - i wonder if there are other exceptions that we need 
> to make. Widening over the stack memory space should be a whitelist, not a 
> blacklist, because we can easily enumerate all stack variables and see which 
> of them can be modified at all from the loop. But until we have that, this 
> looks like a reasonable workaround.


You are right , it's really weird. And the better solution, D36690 
, seems to be blocked.


Repository:
  rC Clang

https://reviews.llvm.org/D45491



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

[PATCH] D44984: [HIP] Add hip file type and codegen for kernel launching

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Frontend/CompilerInvocation.cpp:2109
+  Opts.HIP = true;
+  }
+

yaxunl wrote:
> rjmccall wrote:
> > Why is this done here?  We infer the language mode from the input kind 
> > somewhere else.
> It is usually done through CompilerInvocation::setLangDefaults. However, HIP 
> does not have its own input kind nor is it defined as a language standard. 
> Therefore it cannot use CompilerInvocation::setLangDefaults to set Opts.HIP. 
What are the values of -x if not input kinds or language standards?


https://reviews.llvm.org/D44984



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

[PATCH] D43578: -ftime-report switch support in Clang

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Heh, my e-mail response was eaten along the way. Continued here:

On Sat, Apr 14, 2018 at 1:53 AM, Richard Smith - zygoloid via
Phabricator via cfe-commits  wrote:

> rsmith added a comment.
> 
> So I don't think this patch is reasonable for that reason. I'm also not sure 
> whether this, as is, is addressing a pressing use case -- for Clang 
> developers, existing non-invasive profiling tools (such as linux-perftools) 
> are likely to work a lot better for identifying where in the Clang source 
> code we're spending time. And Clang users typically don't care which function 
> we're in (unless they're filing a bug, where again a profiler is probably a 
> better tool).
> 
> However, I do think we could make `-ftime-report` vastly more useful to Clang 
> users. Specifically, I think the useful, actionable feedback we can give to 
> users would be to tell them which parts of //their source code// are taking a 
> long time to compile. Profiling information that can describe -- for instance 
> -- the time spent instantiating the N slowest templates, or handling the N 
> slowest functions, or evaluating the N slowest constant expressions, or 
> parsing the N slowest `#include`d files, seems like it would be incredibly 
> valuable. To make that work, I think we'll need to teach LLVM's timer 
> infrastructure to properly separate out "self" time from "children" time for 
> timers in the same group, and may need other infrastructure improvements.

I disagreed up until the last paragraph :)

That's exactly the crux of what most users need to know -- which parts
of my source code are causing the biggest build slow-down? The summary
information from -ftime-report can give a hint, but a detailed
breakdown would of course be great!


https://reviews.llvm.org/D43578



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

Re: [PATCH] D43578: -ftime-report switch support in Clang

[Kim Gräsman via cfe-commits]

Hi Richard,

On Sat, Apr 14, 2018 at 1:53 AM, Richard Smith - zygoloid via
Phabricator via cfe-commits  wrote:
> rsmith added a comment.
>
> So I don't think this patch is reasonable for that reason. I'm also not sure 
> whether this, as is, is addressing a pressing use case -- for Clang 
> developers, existing non-invasive profiling tools (such as linux-perftools) 
> are likely to work a lot better for identifying where in the Clang source 
> code we're spending time. And Clang users typically don't care which function 
> we're in (unless they're filing a bug, where again a profiler is probably a 
> better tool).
>
> However, I do think we could make `-ftime-report` vastly more useful to Clang 
> users. Specifically, I think the useful, actionable feedback we can give to 
> users would be to tell them which parts of //their source code// are taking a 
> long time to compile. Profiling information that can describe -- for instance 
> -- the time spent instantiating the N slowest templates, or handling the N 
> slowest functions, or evaluating the N slowest constant expressions, or 
> parsing the N slowest `#include`d files, seems like it would be incredibly 
> valuable. To make that work, I think we'll need to teach LLVM's timer 
> infrastructure to properly separate out "self" time from "children" time for 
> timers in the same group, and may need other infrastructure improvements.

I disagreed up until the last paragraph :)

That's exactly the crux of what most users need to know -- which parts
of my source code are causing the biggest build slow-down? The summary
information from -ftime-report can give a hint, but a detailed
breakdown would of course be great!

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

[PATCH] D43578: -ftime-report switch support in Clang

[Andrew V. Tischenko via Phabricator via cfe-commits]

avt77 added a comment.

In https://reviews.llvm.org/D43578#1067768, @rsmith wrote:

> Last time I looked at doing this, I found that LLVM's timer infrastructure 
> was fundamentally unsuitable for adding timers like these to Clang.


Thank you for this answer. As I understand we should close both this review and 
https://reviews.llvm.org/D45619 but you'd like to see something like MS has 
here , 
right?


https://reviews.llvm.org/D43578



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

[PATCH] D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature

[Andrew V. Tischenko via Phabricator via cfe-commits]

avt77 added inline comments.



Comment at: lib/Basic/FrontendTiming.cpp:18
+
+bool FrontendTimesIsEnabled = false;
+

efriedma wrote:
> Why is this in lib/Basic, when the declaration is in include/clang/Frontend/?
Because this library is being linked to all others and as result this global 
variable could be seen in any Clang library w/o any changes in config files. Or 
you mean it should be moved from Frontend? But where? It's a frontend feature 
that's why I put its declaration there.


https://reviews.llvm.org/D45619



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