Re: [clang-tools-extra] r347520 - A bit of AST matcher cleanup, NFC.

[Aaron Ballman via cfe-commits]

On Sat, Nov 24, 2018 at 9:43 PM Alexander Kornienko via cfe-commits
 wrote:
>
> Author: alexfh
> Date: Sat Nov 24 18:41:01 2018
> New Revision: 347520
>
> URL: http://llvm.org/viewvc/llvm-project?rev=347520&view=rev
> Log:
> A bit of AST matcher cleanup, NFC.
>
> Removed the uses of the allOf() matcher inside node matchers that are implicit
> allOf(). Replaced uses of allOf() with the explicit node matcher where it 
> makes
> matchers more readable. Replace anyOf(hasName(), hasName(), ...) with the more
> efficient and readable hasAnyName().

Would it make sense to have a check under the llvm module for this
coding pattern?

~Aaron

>
> Modified:
> clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
> 
> clang-tools-extra/trunk/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
> clang-tools-extra/trunk/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
> 
> clang-tools-extra/trunk/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp
> clang-tools-extra/trunk/clang-tidy/bugprone/SuspiciousEnumUsageCheck.cpp
> clang-tools-extra/trunk/clang-tidy/bugprone/UseAfterMoveCheck.cpp
> 
> clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.cpp
> clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
> 
> clang-tools-extra/trunk/clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp
> clang-tools-extra/trunk/clang-tidy/fuchsia/TrailingReturnCheck.cpp
> clang-tools-extra/trunk/clang-tidy/google/OverloadedUnaryAndCheck.cpp
> clang-tools-extra/trunk/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
> clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
> clang-tools-extra/trunk/clang-tidy/hicpp/SignedBitwiseCheck.cpp
> 
> clang-tools-extra/trunk/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
> clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp
> clang-tools-extra/trunk/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
> clang-tools-extra/trunk/clang-tidy/modernize/UseAutoCheck.cpp
> clang-tools-extra/trunk/clang-tidy/modernize/UseEqualsDefaultCheck.cpp
> 
> clang-tools-extra/trunk/clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp
> 
> clang-tools-extra/trunk/clang-tidy/performance/MoveConstructorInitCheck.cpp
> 
> clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
> 
> clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
> clang-tools-extra/trunk/clang-tidy/portability/SIMDIntrinsicsCheck.cpp
> clang-tools-extra/trunk/clang-tidy/readability/IsolateDeclarationCheck.cpp
> 
> clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
> 
> clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
> clang-tools-extra/trunk/clang-tidy/zircon/TemporaryObjectsCheck.cpp
>
> Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp?rev=347520&r1=347519&r2=347520&view=diff
> ==
> --- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp (original)
> +++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp Sat Nov 24 
> 18:41:01 2018
> @@ -450,8 +450,8 @@ void ChangeNamespaceTool::registerMatche
>typeLoc(IsInMovedNs,
>loc(qualType(hasDeclaration(DeclMatcher.bind("from_decl",
>unless(anyOf(hasParent(typeLoc(loc(qualType(
> -   allOf(hasDeclaration(DeclMatcher),
> - 
> unless(templateSpecializationType())),
> +   hasDeclaration(DeclMatcher),
> +   unless(templateSpecializationType()),
> hasParent(nestedNameSpecifierLoc()),
> hasAncestor(isImplicit()),
> hasAncestor(UsingShadowDeclInClass),
> @@ -505,13 +505,12 @@ void ChangeNamespaceTool::registerMatche
>  hasAncestor(namespaceDecl(isAnonymous())),
>  hasAncestor(cxxRecordDecl(,
> hasParent(namespaceDecl()));
> -  Finder->addMatcher(
> -  expr(allOf(hasAncestor(decl().bind("dc")), IsInMovedNs,
> - unless(hasAncestor(isImplicit())),
> - anyOf(callExpr(callee(FuncMatcher)).bind("call"),
> -   declRefExpr(to(FuncMatcher.bind("func_decl")))
> -   .bind("func_ref",
> -  this);
> +  Finder->addMatcher(expr(hasAncestor(decl().bind("dc")), IsInMovedNs,
> +  unless(hasAncestor(isImplicit())),
> +  anyOf(callExpr(callee(FuncMatcher)).bind("call"),
> +
> declRefExpr(to(FuncMatche

[PATCH] D54878: [clangd] NFC: Eliminate the unused variable warning.

[Henry Wong via Phabricator via cfe-commits]

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

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54878

Files:
  clangd/AST.cpp


Index: clangd/AST.cpp
===
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -95,11 +95,11 @@
 return Out.str();
   }
   // The name was empty, so present an anonymous entity.
-  if (auto *NS = llvm::dyn_cast(&ND))
+  if (isa(&ND))
 return "(anonymous namespace)";
   if (auto *Cls = llvm::dyn_cast(&ND))
 return ("(anonymous " + Cls->getKindName() + ")").str();
-  if (auto *En = llvm::dyn_cast(&ND))
+  if (isa(&ND))
 return "(anonymous enum)";
   return "(anonymous)";
 }


Index: clangd/AST.cpp
===
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -95,11 +95,11 @@
 return Out.str();
   }
   // The name was empty, so present an anonymous entity.
-  if (auto *NS = llvm::dyn_cast(&ND))
+  if (isa(&ND))
 return "(anonymous namespace)";
   if (auto *Cls = llvm::dyn_cast(&ND))
 return ("(anonymous " + Cls->getKindName() + ")").str();
-  if (auto *En = llvm::dyn_cast(&ND))
+  if (isa(&ND))
 return "(anonymous enum)";
   return "(anonymous)";
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r347520 - A bit of AST matcher cleanup, NFC.

[Alexander Kornienko via cfe-commits]

On Sun, Nov 25, 2018 at 1:52 PM Aaron Ballman 
wrote:

> On Sat, Nov 24, 2018 at 9:43 PM Alexander Kornienko via cfe-commits
>  wrote:
> >
> > Author: alexfh
> > Date: Sat Nov 24 18:41:01 2018
> > New Revision: 347520
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=347520&view=rev
> > Log:
> > A bit of AST matcher cleanup, NFC.
> >
> > Removed the uses of the allOf() matcher inside node matchers that are
> implicit
> > allOf(). Replaced uses of allOf() with the explicit node matcher where
> it makes
> > matchers more readable. Replace anyOf(hasName(), hasName(), ...) with
> the more
> > efficient and readable hasAnyName().
>
> Would it make sense to have a check under the llvm module for this
> coding pattern?
>

Probably yes, but I wouldn't be too optimistic about the positive impact of
that check compared to the cost of creating and supporting it.

If someone has a large enough chunk of time they could devote to improving
clang-tidy performance, there was an interesting idea about optimizing AST
matchers at runtime. E.g. currently `unless(isInTemplateInstantiation())`
matchers go up the ancestor chain, but a more efficient implementation
could just skip traversal of template instantiations for the matchers that
have the `unless(isInTemplateInstantiation())` constraint. To implement
this, the matchers framework would need to analyze the matchers and rewrite
them (instead of just passing control to them).


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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Sorry for the huge noise! It took me a while to see what is the problem.


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

https://reviews.llvm.org/D53076



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

[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a subscriber: jingham.
a_sidorin added a comment.

Hi Gabor,

I think it is a good patch with a nice test set. There are some mine comments 
inline. I'd also like to have LLDB guys opinion on it.




Comment at: lib/AST/ASTImporter.cpp:7605
+
+ASTImporter::ASTImporter(ASTImporterLookupTable *LookupTable,
+ ASTContext &ToContext, FileManager &ToFileManager,

It's better to make `LookupTable` an optional parameter of the previous ctor 
and remove this one.



Comment at: lib/AST/ASTImporter.cpp:7657
+  } else {
+// FIXME Can we remove this kind of lookup?
+// Or lldb really needs this C/C++ lookup?

@shafik @jingham  Could you please address this question?



Comment at: lib/AST/ASTImporterLookupTable.cpp:52
+
+} // namespace unnamed
+

`// anonymous namespace` or just `// namespace`.



Comment at: lib/AST/ASTImporterLookupTable.cpp:84
+  DeclContext *DC = ND->getDeclContext()->getPrimaryContext();
+  remove(DC, ND);
+  DeclContext *ReDC = DC->getRedeclContext()->getPrimaryContext();

Should we remove DC-related entry if it is empty after deletion?



Comment at: lib/AST/ASTImporterLookupTable.cpp:95
+return {};
+  const auto &FoundNameMap = DCI->second;
+  auto NamesI = FoundNameMap.find(Name);

Could you please add newlines after `if`s?



Comment at: lib/AST/ASTImporterLookupTable.cpp:121
+DeclContext* DC = Entry.first;
+std::string Primary = DC->getPrimaryContext() ? " primary " : " ";
+llvm::errs() << "== DC: " << cast(DC) << Primary <<  "\n";

1. StringRef
2. Do we need a space before newline?



Comment at: lib/AST/ASTImporterLookupTable.cpp:122
+std::string Primary = DC->getPrimaryContext() ? " primary " : " ";
+llvm::errs() << "== DC: " << cast(DC) << Primary <<  "\n";
+dump(DC);

Two spaces before "\n".



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:258
+TranslationUnitDecl *ToTU) {
+  if (LookupTable)
+return;

```if (!LookupTable)
  LookupTable = llvm::make_unique(*ToTU); ```
?



Comment at: unittests/AST/ASTImporterTest.cpp:346
+assert(ToTU);
+if (LookupTablePtr)
+  return;

`if (!LookupTablePtr) ...`



Comment at: unittests/AST/ASTImporterTest.cpp:3843
+TEST_P(ASTImporterLookupTableTest, EmptyCode) {
+  auto *ToTU = getToTuDecl( "", Lang_CXX);
+  ASTImporterLookupTable LT(*ToTU);

Redundant space after paren.



Comment at: unittests/AST/ASTImporterTest.cpp:3845
+  ASTImporterLookupTable LT(*ToTU);
+}
+

Could you please explain what does this test do?



Comment at: unittests/AST/ASTImporterTest.cpp:3889
+if (ND->getDeclName() == Name)
+  Found = ND;
+}

Do we need break/early return here?



Comment at: unittests/AST/ASTImporterTest.cpp:3930
+
+  auto FindInDeclListOfDC = [](DeclContext *DC, DeclarationName Name) {
+Decl *Found = nullptr;

This lambda is the same as in previous func so it's better to extract it into a 
helper.



Comment at: unittests/AST/ASTImporterTest.cpp:4020
+  const RecordDecl *RD = getRecordDeclOfFriend(FriendD);
+  auto *Y = FirstDeclMatcher().match(ToTU, 
cxxRecordDecl(hasName("Y")));
+

This line exceeds 80 chars.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53708



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

[PATCH] D53818: [ASTImporter] Changed use of Import to Import_New in ASTImporter.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM. Thank you for addressing my questions!


Repository:
  rC Clang

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

https://reviews.llvm.org/D53818



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

[PATCH] D53818: [ASTImporter] Changed use of Import to Import_New in ASTImporter.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D53818#1307321 , @a_sidorin wrote:

> LGTM. Thank you for addressing my questions!


Hi Alexei,

Could you please also take a look on that patch which this one depends on? 
https://reviews.llvm.org/D53751
I think the changes of that patch are included in this one too. Thank you.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53818



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

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

Szelethus added a comment.

I think the reason why the printed message was either along the lines of "this 
is 0" and "this is non-0", is that we don't necessarily know what constraint 
solver we're using, and adding more non-general code `BugReporter` is most 
probably a bad approach. How about we tinker with `ContraintManager` a little 
more, maybe add an `explainValueOfVarDecl`, that would return the value of a 
variable in a given `ProgramState`? We could implement it for 
`RangeConstraintSolver` (essentially move the code you already wrote there), 
and leave a `TODO` with a super generic implementation for Z3.

Although, I can't quite write an essay on top of my head about golden rules of 
constraint solving, so take my advice with a grain of salt.




Comment at: 
include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:164
 class ConditionBRVisitor final : public BugReporterVisitor {
+  bool IsAssuming;
+

We only get to know what this field is for while reading the actual 
implementation, please add comments.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1892-1920
+  const auto CurrentCR = N->getState()->get();
+  const auto PredCR = PredState->get();
+  const auto PredPredCR = PredPredState->get();
+
+  // We only want to check BlockEdges once so we have to determine if the 
change
+  // of the range information is not happened because of dead symbols.
+  //

`ConstraintRange`, as far as I know, is the data  `RangedConstraintManager` 
stores in the GDM. What if we're using a different constraint solver? I think 
it'd be better to take the more abstract approach, extend `ConstraintManager`'s 
interface with an `isEqual` or `operator==` pure virtual method, and implement 
it within it's descendants.


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

https://reviews.llvm.org/D53076



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

r347527 - [CodeGen] translate MS rotate builtins to LLVM funnel-shift intrinsics

[Sanjay Patel via cfe-commits]

Author: spatel
Date: Sun Nov 25 09:53:16 2018
New Revision: 347527

URL: http://llvm.org/viewvc/llvm-project?rev=347527&view=rev
Log:
[CodeGen] translate MS rotate builtins to LLVM funnel-shift intrinsics

This was originally part of:
D50924

and should resolve PR37387:
https://bugs.llvm.org/show_bug.cgi?id=37387

...but it was reverted because some bots using a gcc host compiler 
would crash for unknown reasons with this included in the patch. 
Trying again now to see if that's still a problem.

Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c

Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=347527&r1=347526&r2=347527&view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Sun Nov 25 09:53:16 2018
@@ -1820,46 +1820,6 @@ RValue CodeGenFunction::EmitBuiltinExpr(
  "cast");
 return RValue::get(Result);
   }
-  case Builtin::BI_rotr8:
-  case Builtin::BI_rotr16:
-  case Builtin::BI_rotr:
-  case Builtin::BI_lrotr:
-  case Builtin::BI_rotr64: {
-Value *Val = EmitScalarExpr(E->getArg(0));
-Value *Shift = EmitScalarExpr(E->getArg(1));
-
-llvm::Type *ArgType = Val->getType();
-Shift = Builder.CreateIntCast(Shift, ArgType, false);
-unsigned ArgWidth = ArgType->getIntegerBitWidth();
-Value *Mask = llvm::ConstantInt::get(ArgType, ArgWidth - 1);
-
-Value *RightShiftAmt = Builder.CreateAnd(Shift, Mask);
-Value *RightShifted = Builder.CreateLShr(Val, RightShiftAmt);
-Value *LeftShiftAmt = Builder.CreateAnd(Builder.CreateNeg(Shift), Mask);
-Value *LeftShifted = Builder.CreateShl(Val, LeftShiftAmt);
-Value *Result = Builder.CreateOr(LeftShifted, RightShifted);
-return RValue::get(Result);
-  }
-  case Builtin::BI_rotl8:
-  case Builtin::BI_rotl16:
-  case Builtin::BI_rotl:
-  case Builtin::BI_lrotl:
-  case Builtin::BI_rotl64: {
-Value *Val = EmitScalarExpr(E->getArg(0));
-Value *Shift = EmitScalarExpr(E->getArg(1));
-
-llvm::Type *ArgType = Val->getType();
-Shift = Builder.CreateIntCast(Shift, ArgType, false);
-unsigned ArgWidth = ArgType->getIntegerBitWidth();
-Value *Mask = llvm::ConstantInt::get(ArgType, ArgWidth - 1);
-
-Value *LeftShiftAmt = Builder.CreateAnd(Shift, Mask);
-Value *LeftShifted = Builder.CreateShl(Val, LeftShiftAmt);
-Value *RightShiftAmt = Builder.CreateAnd(Builder.CreateNeg(Shift), Mask);
-Value *RightShifted = Builder.CreateLShr(Val, RightShiftAmt);
-Value *Result = Builder.CreateOr(LeftShifted, RightShifted);
-return RValue::get(Result);
-  }
   case Builtin::BI__builtin_unpredictable: {
 // Always return the argument of __builtin_unpredictable. LLVM does not
 // handle this builtin. Metadata for this builtin should be added directly
@@ -1918,12 +1878,22 @@ RValue CodeGenFunction::EmitBuiltinExpr(
   case Builtin::BI__builtin_rotateleft16:
   case Builtin::BI__builtin_rotateleft32:
   case Builtin::BI__builtin_rotateleft64:
+  case Builtin::BI_rotl8: // Microsoft variants of rotate left
+  case Builtin::BI_rotl16:
+  case Builtin::BI_rotl:
+  case Builtin::BI_lrotl:
+  case Builtin::BI_rotl64:
 return emitRotate(E, false);
 
   case Builtin::BI__builtin_rotateright8:
   case Builtin::BI__builtin_rotateright16:
   case Builtin::BI__builtin_rotateright32:
   case Builtin::BI__builtin_rotateright64:
+  case Builtin::BI_rotr8: // Microsoft variants of rotate right
+  case Builtin::BI_rotr16:
+  case Builtin::BI_rotr:
+  case Builtin::BI_lrotr:
+  case Builtin::BI_rotr64:
 return emitRotate(E, true);
 
   case Builtin::BI__builtin_constant_p: {

Modified: cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c?rev=347527&r1=347526&r2=347527&view=diff
==
--- cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c (original)
+++ cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c Sun Nov 25 09:53:16 2018
@@ -30,66 +30,36 @@ unsigned char test_rotl8(unsigned char v
   return _rotl8(value, shift);
 }
 // CHECK: i8 @test_rotl8
-// CHECK:   [[LSHIFT:%[0-9]+]] = and i8 [[SHIFT:%[0-9]+]], 7
-// CHECK:   [[HIGH:%[0-9]+]] = shl i8 [[VALUE:%[0-9]+]], [[LSHIFT]]
-// CHECK:   [[NEGATE:%[0-9]+]] = sub i8 0, [[SHIFT]]
-// CHECK:   [[RSHIFT:%[0-9]+]] = and i8 [[NEGATE]], 7
-// CHECK:   [[LOW:%[0-9]+]] = lshr i8 [[VALUE]], [[RSHIFT]]
-// CHECK:   [[RESULT:%[0-9]+]] = or i8 [[HIGH]], [[LOW]]
-// CHECK:   ret i8 [[RESULT]]
-// CHECK  }
+// CHECK:   [[R:%.*]] = call i8 @llvm.fshl.i8(i8 [[X:%.*]], i8 [[X]], i8 
[[Y:%.*]])
+// CHECK:   ret i8 [[R]]
 
 unsigned short test_rotl16(unsigned short value, unsigned char shift) {
   return _rotl16(value, shift);
 }
 // CHECK: i16 @

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,

The change looks mostly Ok but there are some comments inline.




Comment at: include/clang/AST/DeclContextInternals.h:128
+DeclsTy &Vec = *getAsVector();
+DeclsTy::iterator I = std::find(Vec.begin(), Vec.end(), D);
+return I != Vec.end();

`auto I = llvm::find(Vec, D)`?



Comment at: lib/AST/ASTImporter.cpp:2633
+// struct/union and in case of anonymous structs.  Would be false
+// becasue there may be several anonymous/unnamed structs in a class.
+// E.g. these are both valid:

because (typo)



Comment at: lib/AST/ASTImporter.cpp:2644
 
-PrevDecl = FoundRecord;
-
-if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
-  if ((SearchName && !D->isCompleteDefinition() && !IsFriendTemplate)
-  || (D->isCompleteDefinition() &&
-  D->isAnonymousStructOrUnion()
-== FoundDef->isAnonymousStructOrUnion())) {
-// The record types structurally match, or the "from" translation
-// unit only had a forward declaration anyway; call it the same
-// function.
+if (IsStructuralMatch(D, FoundRecord)) {
+  RecordDecl *FoundDef = FoundRecord->getDefinition();

IsStructuralMatch check will be repeated if (!SearchName && IsStructuralMatch). 
Is it expected behaviour?



Comment at: lib/AST/ASTImporter.cpp:2667
   ConflictingDecls.push_back(FoundDecl);
-}
+} // for
 

Szelethus wrote:
> Hah. We should do this more often.
Unfortunately, it is a clear sign that we have to simplify the function. It's 
better to leave a FIXME instead.



Comment at: lib/AST/ASTImporter.cpp:2821
 
-  Importer.MapImported(D, D2);
 

Are these lines removed due to move of MapImported into Create helper?



Comment at: lib/AST/ASTImporter.cpp:4991
 if (IsStructuralMatch(D, FoundTemplate)) {
-  if (!IsFriend) {
-Importer.MapImported(D->getTemplatedDecl(),
- FoundTemplate->getTemplatedDecl());
-return Importer.MapImported(D, FoundTemplate);
+  ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+  if (D->isThisDeclarationADefinition() && TemplateWithDef) {

Space after '*'?



Comment at: unittests/AST/ASTImporterTest.cpp:3794
+
+TEST_P(ImportFriendClasses, DeclsFromFriendsShouldBeInRedeclChains2) {
+  Decl *From, *To;

Will `DeclsFromFriendsShouldBeInRedeclChains` without number appear in another 
patch?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a subscriber: spyffe.
a_sidorin added a comment.

Hi Balazs,
 The changes look mostly fine to me. I think they can be accepted after some 
minor stylish fixes.

Hi Shafik,

> I think these changes make sense at a high level but I am not sure about the 
> refactoring strategy. I am especially concerned we may end up in place where 
> all the effected users of the API don't get updated and we are stuck with 
> this parallel API.

As I understand, these changes are done exactly in order to perform a seamless 
transition into a new error-checking API. This was discussed in both cfe-dev 
and lldb-dev: https://lists.llvm.org/pipermail/cfe-dev/2018-April/057771.html.
It looks like the patches for the final transition will be ready soon. 
@balazske, @martong Am I correct?

> Tagging in @rsmith since he has reviewed a lot of recent changes involving 
> ASTImpoter that I have seen recently and he will have a better feeling for 
> how these types of refactoring on handled on the clang side. I am mostly 
> focused on the lldb side but care about the ASTImporter since we depend on it.

Review from @rsmith is extremely appreciated; however, me and @spyffe have 
reviewed most ASTImporter-related patches in the last two years. ASTImporter 
itself doesn't have any use in clang frontend. It was an LLDB bridge only until 
CSA started to use it too.




Comment at: include/clang/AST/ASTImporter.h:244
+/// context, or the import error.
+llvm::Expected Import_New(NestedNameSpecifier 
*FromNNS);
+// FIXME: Remove this version.

Please split this line and the changed line below.



Comment at: lib/AST/ASTImporter.cpp:7882
 
+Expected ASTImporter::Import_New(NestedNameSpecifier 
*FromNNS) {
+  NestedNameSpecifier *ToNNS = Import(FromNNS);

Looks like this line needs to be split too.



Comment at: lib/AST/ASTImporter.cpp:8254
 
+Expected ASTImporter::Import_New(const CXXBaseSpecifier 
*From) {
+  CXXBaseSpecifier *To = Import(From);

... and this one too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53751



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added subscribers: aaron.ballman, rsmith.
a_sidorin added a comment.

Please note that changes in AST lib will require AST code owners' approval. 
@rsmith, @aaron.ballman could you please take a look?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

More constants are always welcome.




Comment at: lib/AST/ExternalASTMerger.cpp:154
   ToContainer->setHasExternalLexicalStorage();
-  ToContainer->setMustBuildLookupTable();
+  ToContainer->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToContainer));

Do we have to do the same for NamespaceDecls?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53755



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

[PATCH] D53693: [ASTImporter] Typedef import brings in the complete type

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D53693



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/DeclContextInternals.h:128
+DeclsTy &Vec = *getAsVector();
+DeclsTy::iterator I = std::find(Vec.begin(), Vec.end(), D);
+return I != Vec.end();

a_sidorin wrote:
> `auto I = llvm::find(Vec, D)`?
I'd go with `return llvm::is_contained(Vec, D);`



Comment at: include/clang/AST/DeclContextInternals.h:125
 
+  bool containsInVector(NamedDecl *D) {
+assert(getAsVector());

`const NamedDecl *D`



Comment at: include/clang/AST/DeclContextInternals.h:126
+  bool containsInVector(NamedDecl *D) {
+assert(getAsVector());
+DeclsTy &Vec = *getAsVector();

Please add a string literal to the assert explaining what is being checked here.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:1169
+///   matches 'a'.
+extern const internal::VariadicDynCastAllOfMatcher
+indirectFieldDecl;

Be sure to update Registry.cpp and regenerate the AST matcher documentation by 
running clang\docs\tools\dump_ast_matchers.py.

This change feels orthogonal to the rest of the patch; perhaps it should be 
split out into its own patch?



Comment at: lib/AST/ASTImporter.cpp:4991
 if (IsStructuralMatch(D, FoundTemplate)) {
-  if (!IsFriend) {
-Importer.MapImported(D->getTemplatedDecl(),
- FoundTemplate->getTemplatedDecl());
-return Importer.MapImported(D, FoundTemplate);
+  ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+  if (D->isThisDeclarationADefinition() && TemplateWithDef) {

a_sidorin wrote:
> Space after '*'?
Space before the asterisk. ;-)



Comment at: lib/AST/ASTImporter.cpp:2729
+
+const bool IsFriend = D->isInIdentifierNamespace(Decl::IDNS_TagFriend);
+if (LexicalDC != DC && IsFriend) {

Drop the top-level `const` qualifier.



Comment at: lib/AST/ASTImporter.cpp:2730
+const bool IsFriend = D->isInIdentifierNamespace(Decl::IDNS_TagFriend);
+if (LexicalDC != DC && IsFriend) {
+  DC->makeDeclVisibleInContext(D2);

Elide braces; I'd probably sink the `IsFriend` into here since it's not used 
elsewhere.



Comment at: lib/AST/ASTImporter.cpp:5064
+if (!ToTemplated->getPreviousDecl()) {
+  auto *PrevTemplated =
+  FoundByLookup->getTemplatedDecl()->getMostRecentDecl();

Do not use `auto` here as the type is not spelled out in the initialization.



Comment at: lib/AST/ASTImporter.cpp:5073
+
+  if (LexicalDC != DC && IsFriend) {
+DC->makeDeclVisibleInContext(D2);

Elide braces.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

r347529 - [CodeComplete] Simplify CodeCompleteConsumer.cpp, NFC

[Fangrui Song via cfe-commits]

Author: maskray
Date: Sun Nov 25 12:57:05 2018
New Revision: 347529

URL: http://llvm.org/viewvc/llvm-project?rev=347529&view=rev
Log:
[CodeComplete] Simplify CodeCompleteConsumer.cpp, NFC

Use range-based for loops
Use XStr.compare(YStr) < 0
Format misaligned code

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

Modified: cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp?rev=347529&r1=347528&r2=347529&view=diff
==
--- cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp (original)
+++ cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp Sun Nov 25 12:57:05 2018
@@ -272,23 +272,18 @@ CodeCompletionString::Chunk::CreateResul
   return Chunk(CK_ResultType, ResultType);
 }
 
-CodeCompletionString::Chunk
-CodeCompletionString::Chunk::CreateCurrentParameter(
-const char *CurrentParameter) {
+CodeCompletionString::Chunk 
CodeCompletionString::Chunk::CreateCurrentParameter(
+const char *CurrentParameter) {
   return Chunk(CK_CurrentParameter, CurrentParameter);
 }
 
-CodeCompletionString::CodeCompletionString(const Chunk *Chunks,
-   unsigned NumChunks,
-   unsigned Priority,
-   CXAvailabilityKind Availability,
-   const char **Annotations,
-   unsigned NumAnnotations,
-   StringRef ParentName,
-   const char *BriefComment)
-: NumChunks(NumChunks), NumAnnotations(NumAnnotations),
-  Priority(Priority), Availability(Availability),
-  ParentName(ParentName), BriefComment(BriefComment) {
+CodeCompletionString::CodeCompletionString(
+const Chunk *Chunks, unsigned NumChunks, unsigned Priority,
+CXAvailabilityKind Availability, const char **Annotations,
+unsigned NumAnnotations, StringRef ParentName, const char *BriefComment)
+: NumChunks(NumChunks), NumAnnotations(NumAnnotations), Priority(Priority),
+  Availability(Availability), ParentName(ParentName),
+  BriefComment(BriefComment) {
   assert(NumChunks <= 0x);
   assert(NumAnnotations <= 0x);
 
@@ -296,7 +291,8 @@ CodeCompletionString::CodeCompletionStri
   for (unsigned I = 0; I != NumChunks; ++I)
 StoredChunks[I] = Chunks[I];
 
-  const char **StoredAnnotations = reinterpret_cast(StoredChunks + NumChunks);
+  const char **StoredAnnotations =
+  reinterpret_cast(StoredChunks + NumChunks);
   for (unsigned I = 0; I != NumAnnotations; ++I)
 StoredAnnotations[I] = Annotations[I];
 }
@@ -307,7 +303,7 @@ unsigned CodeCompletionString::getAnnota
 
 const char *CodeCompletionString::getAnnotation(unsigned AnnotationNr) const {
   if (AnnotationNr < NumAnnotations)
-return reinterpret_cast(end())[AnnotationNr];
+return reinterpret_cast(end())[AnnotationNr];
   else
 return nullptr;
 }
@@ -316,27 +312,33 @@ std::string CodeCompletionString::getAsS
   std::string Result;
   llvm::raw_string_ostream OS(Result);
 
-  for (iterator C = begin(), CEnd = end(); C != CEnd; ++C) {
-switch (C->Kind) {
-case CK_Optional: OS << "{#" << C->Optional->getAsString() << "#}"; break;
-case CK_Placeholder: OS << "<#" << C->Text << "#>"; break;
-
+  for (const Chunk &C : *this) {
+switch (C.Kind) {
+case CK_Optional:
+  OS << "{#" << C.Optional->getAsString() << "#}";
+  break;
+case CK_Placeholder:
+  OS << "<#" << C.Text << "#>";
+  break;
 case CK_Informative:
 case CK_ResultType:
-  OS << "[#" << C->Text << "#]";
+  OS << "[#" << C.Text << "#]";
+  break;
+case CK_CurrentParameter:
+  OS << "<#" << C.Text << "#>";
+  break;
+default:
+  OS << C.Text;
   break;
-
-case CK_CurrentParameter: OS << "<#" << C->Text << "#>"; break;
-default: OS << C->Text; break;
 }
   }
   return OS.str();
 }
 
 const char *CodeCompletionString::getTypedText() const {
-  for (iterator C = begin(), CEnd = end(); C != CEnd; ++C)
-if (C->Kind == CK_TypedText)
-  return C->Text;
+  for (const Chunk &C : *this)
+if (C.Kind == CK_TypedText)
+  return C.Text;
 
   return nullptr;
 }
@@ -371,7 +373,7 @@ StringRef CodeCompletionTUInfo::getParen
   // Find the interesting names.
   SmallVector Contexts;
   while (DC && !DC->isFunctionOrMethod()) {
-if (const NamedDecl *ND = dyn_cast(DC)) {
+if (const auto *ND = dyn_cast(DC)) {
   if (ND->getIdentifier())
 Contexts.push_back(DC);
 }
@@ -390,11 +392,11 @@ StringRef CodeCompletionTUInfo::getParen
 OS << "::";
   }
 
-  const DeclContext *CurDC = Contexts[I-1];
-  if (const ObjCCategoryImplDecl *CatImpl = 
dyn_cast(CurDC))
+  const DeclContext *CurDC = Contexts[I - 1];
+  i

[PATCH] D54880: Ignore gcc's stack-clash-protection flag

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev created this revision.
v.g.vassilev added reviewers: rsmith, bruno.

Patch by Mattias Ellert!


Repository:
  rC Clang

https://reviews.llvm.org/D54880

Files:
  include/clang/Driver/Options.td


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -2989,6 +2989,7 @@
 Group;
 defm spec_constr_count : BooleanFFlag<"spec-constr-count">, 
Group;
 defm stack_check : BooleanFFlag<"stack-check">, Group;
+defm stack_clash_protection : BooleanFFlag<"stack-clash-protection">, 
Group;
 defm strength_reduce :
 BooleanFFlag<"strength-reduce">, 
Group;
 defm tls_model : BooleanFFlag<"tls-model">, Group;


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -2989,6 +2989,7 @@
 Group;
 defm spec_constr_count : BooleanFFlag<"spec-constr-count">, Group;
 defm stack_check : BooleanFFlag<"stack-check">, Group;
+defm stack_clash_protection : BooleanFFlag<"stack-clash-protection">, Group;
 defm strength_reduce :
 BooleanFFlag<"strength-reduce">, Group;
 defm tls_model : BooleanFFlag<"tls-model">, Group;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

@Szelethus thanks you for the comments!

In D53076#1307336 , @Szelethus wrote:

> I think the reason why the printed message was either along the lines of 
> "this is 0" and "this is non-0", is that we don't necessarily know what 
> constraint solver we're using, and adding more non-general code `BugReporter` 
> is most probably a bad approach.


It was an unimplemented feature to write out known stuff. There is no 
constraint solving, it is rather constant value dumping.

> How about we tinker with `ContraintManager` a little more, maybe add an 
> `explainValueOfVarDecl`, that would return the value of a variable in a given 
> `ProgramState`? We could implement it for `RangeConstraintSolver` 
> (essentially move the code you already wrote there), and leave a `TODO` with 
> a super generic implementation for Z3.

I agree with @george.karpenkov, we want to have the smallest scope.




Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1892-1920
+  const auto CurrentCR = N->getState()->get();
+  const auto PredCR = PredState->get();
+  const auto PredPredCR = PredPredState->get();
+
+  // We only want to check BlockEdges once so we have to determine if the 
change
+  // of the range information is not happened because of dead symbols.
+  //

Szelethus wrote:
> `ConstraintRange`, as far as I know, is the data  `RangedConstraintManager` 
> stores in the GDM. What if we're using a different constraint solver? I think 
> it'd be better to take the more abstract approach, extend 
> `ConstraintManager`'s interface with an `isEqual` or `operator==` pure 
> virtual method, and implement it within it's descendants.
Because we only operate in the `ConditionBRVisitor` it would be useless for now.


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

https://reviews.llvm.org/D53076



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

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

[Russell McClellan via Phabricator via cfe-commits]

russellmcc created this revision.
russellmcc added a reviewer: djasper.
Herald added a subscriber: cfe-commits.

When clang-format is set to always use tabs (with tab width 4), when asked to 
format line 3 (using the `-lines=3:3` flag):

  int f() {
  int a;
  foobar();
  int b;
  }

We would expect clang-format to not modify the leading whitespace on line 4.  
However, it does - see the new test.  This is because the whitespace manager is 
called to replace the whitespace before the first token of line 4.  This is 
necessary to edit the number of new lines before line 4, and to edit the 
trailing whitespace on line 3.  I've added a flag to `replaceWhitespace` that 
allows it to not edit the leading whitespace, and only edit the whitespace up 
to the last newline.

We ran into this when trying to integrate clang-format into our CI to ensure no 
new formatting diffs were introduced in a patch.  With the behavior without 
this patch, the number of affected lines grew each time clang-format was run, 
so running clang-format locally was not enough to ensure that CI would pass.


Repository:
  rC Clang

https://reviews.llvm.org/D54881

Files:
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineFormatter.h
  lib/Format/WhitespaceManager.cpp
  lib/Format/WhitespaceManager.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -36,12 +36,12 @@
 SC_DoNotCheck
   };
 
-  std::string format(llvm::StringRef Code,
- const FormatStyle &Style = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+  std::string formatRange(llvm::StringRef Code, tooling::Range Range,
+  const FormatStyle &Style = getLLVMStyle(),
+  StatusCheck CheckComplete = SC_ExpectComplete) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
-std::vector Ranges(1, tooling::Range(0, Code.size()));
+std::vector Ranges(1, std::move(Range));
 FormattingAttemptStatus Status;
 tooling::Replacements Replaces =
 reformat(Style, Code, Ranges, "", &Status);
@@ -57,6 +57,13 @@
 return *Result;
   }
 
+  std::string format(llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+return formatRange(Code, tooling::Range(0, Code.size()), Style,
+   CheckComplete);
+  }
+
   FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
 Style.ColumnLimit = ColumnLimit;
 return Style;
@@ -110,6 +117,14 @@
 format(Code, Style, SC_DoNotCheck);
   }
 
+  void verifyWithPrefixAndSuffix(llvm::StringRef Expected, llvm::StringRef Code,
+ llvm::StringRef prefix, llvm::StringRef suffix,
+ const FormatStyle &Style = getLLVMStyle()) {
+EXPECT_EQ(llvm::Twine(prefix + Expected + suffix).str(),
+  formatRange(llvm::Twine(prefix + Code + suffix).str(),
+  tooling::Range(prefix.size(), Code.size()), Style));
+  }
+
   int ReplacementCount;
 };
 
@@ -9141,6 +9156,7 @@
"\t */\n"
"\t int i;\n"
"}"));
+
   Tab.AlignConsecutiveAssignments = true;
   Tab.AlignConsecutiveDeclarations = true;
   Tab.TabWidth = 4;
@@ -9156,6 +9172,18 @@
Tab);
 }
 
+TEST_F(FormatTest, FormattingIndentationDoesNotAffectOtherLines) {
+  FormatStyle Tab = getLLVMStyleWithColumns(42);
+  Tab.TabWidth = 4;
+  Tab.UseTab = FormatStyle::UT_Always;
+  verifyWithPrefixAndSuffix("\tfoobar();", "foobar();",
+"int f() {\nint a;\n", "\nint b;\n}\n",
+Tab);
+  Tab.UseTab = FormatStyle::UT_Never;
+  verifyWithPrefixAndSuffix("foobar();", "\tfoobar();",
+"int f() {\n\tint a;\n", "\n\tint b;\n}\n", Tab);
+}
+
 TEST_F(FormatTest, CalculatesOriginalColumn) {
   EXPECT_EQ("\"qq\\\n"
 "q\"; /* some\n"
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -49,7 +49,8 @@
   /// into tabs and spaces for some format styles.
   void replaceWhitespace(FormatToken &Tok, unsigned Newlines, unsigned Spaces,
  unsigned StartOfTokenColumn,
- bool InPPDirective = false);
+ bool InPPDirective = false,
+ bool OnlyUntilLastNewline = false);
 
   /// Adds information about an unchangeable token's whitespace.
   ///
Index: lib/Format/WhitespaceManager.cpp

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

[Russell McClellan via Phabricator via cfe-commits]

russellmcc updated this revision to Diff 175185.

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

https://reviews.llvm.org/D54881

Files:
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineFormatter.h
  lib/Format/WhitespaceManager.cpp
  lib/Format/WhitespaceManager.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -36,12 +36,12 @@
 SC_DoNotCheck
   };
 
-  std::string format(llvm::StringRef Code,
- const FormatStyle &Style = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+  std::string formatRange(llvm::StringRef Code, tooling::Range Range,
+  const FormatStyle &Style = getLLVMStyle(),
+  StatusCheck CheckComplete = SC_ExpectComplete) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
-std::vector Ranges(1, tooling::Range(0, Code.size()));
+std::vector Ranges(1, std::move(Range));
 FormattingAttemptStatus Status;
 tooling::Replacements Replaces =
 reformat(Style, Code, Ranges, "", &Status);
@@ -57,6 +57,13 @@
 return *Result;
   }
 
+  std::string format(llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+return formatRange(Code, tooling::Range(0, Code.size()), Style,
+   CheckComplete);
+  }
+
   FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
 Style.ColumnLimit = ColumnLimit;
 return Style;
@@ -110,6 +117,14 @@
 format(Code, Style, SC_DoNotCheck);
   }
 
+  void verifyWithPrefixAndSuffix(llvm::StringRef Expected, llvm::StringRef Code,
+ llvm::StringRef prefix, llvm::StringRef suffix,
+ const FormatStyle &Style = getLLVMStyle()) {
+EXPECT_EQ(llvm::Twine(prefix + Expected + suffix).str(),
+  formatRange(llvm::Twine(prefix + Code + suffix).str(),
+  tooling::Range(prefix.size(), Code.size()), Style));
+  }
+
   int ReplacementCount;
 };
 
@@ -9141,6 +9156,7 @@
"\t */\n"
"\t int i;\n"
"}"));
+
   Tab.AlignConsecutiveAssignments = true;
   Tab.AlignConsecutiveDeclarations = true;
   Tab.TabWidth = 4;
@@ -9156,6 +9172,18 @@
Tab);
 }
 
+TEST_F(FormatTest, FormattingIndentationDoesNotAffectOtherLines) {
+  FormatStyle Tab = getLLVMStyleWithColumns(42);
+  Tab.TabWidth = 4;
+  Tab.UseTab = FormatStyle::UT_Always;
+  verifyWithPrefixAndSuffix("\tfoobar();", "foobar();",
+"int f() {\nint a;\n", "\nint b;\n}\n",
+Tab);
+  Tab.UseTab = FormatStyle::UT_Never;
+  verifyWithPrefixAndSuffix("foobar();", "\tfoobar();",
+"int f() {\n\tint a;\n", "\n\tint b;\n}\n", Tab);
+}
+
 TEST_F(FormatTest, CalculatesOriginalColumn) {
   EXPECT_EQ("\"qq\\\n"
 "q\"; /* some\n"
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -49,7 +49,8 @@
   /// into tabs and spaces for some format styles.
   void replaceWhitespace(FormatToken &Tok, unsigned Newlines, unsigned Spaces,
  unsigned StartOfTokenColumn,
- bool InPPDirective = false);
+ bool InPPDirective = false,
+ bool OnlyUntilLastNewline = false);
 
   /// Adds information about an unchangeable token's whitespace.
   ///
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -46,13 +46,23 @@
 void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines,
   unsigned Spaces,
   unsigned StartOfTokenColumn,
-  bool InPPDirective) {
+  bool InPPDirective,
+  bool OnlyUntilLastNewline) {
   if (Tok.Finalized)
 return;
   Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
-  Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange,
-   Spaces, StartOfTokenColumn, Newlines, "", "",
-   InPPDirective && !Tok.IsFirst,
+  auto OriginalWhitespaceRange = Tok.WhitespaceRange;
+
+  if (OnlyUntilLastNewline) {
+OriginalWhitespaceRange.setEnd(
+OriginalWhitespaceRange.getBegin().getLocWithOffset(
+ 

[PATCH] D54379: Add Hurd toolchain support to Clang

[Samuel Thibault via Phabricator via cfe-commits]

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

I'll update the diff according to the comments.




Comment at: lib/Driver/ToolChains/Hurd.cpp:74
+
+  // Similar to the logic for GCC above, if we currently running Clang inside
+  // of the requested system root, add its parent library paths to

aaron.ballman wrote:
> What GCC logic above?
> 
> we currently -> we are currently
That was from Linux.cpp. I have not yet included the gcc pieces above, so the 
comment doesn't make sense yet indeed.



Comment at: lib/Driver/ToolChains/Hurd.cpp:120
+
+  llvm_unreachable("unsupported architecture");
+}

aaron.ballman wrote:
> This doesn't look unreachable to me?
For now it is, we only have x86 architecture for the Hurd. This is like 
Linux::getDynamicLinker which uses llvm_unreachable in the default case.



Comment at: lib/Driver/ToolChains/Hurd.h:31-33
+  virtual std::string computeSysRoot() const;
+
+  virtual std::string getDynamicLinker(const llvm::opt::ArgList &Args) const;

aaron.ballman wrote:
> Why are these virtual functions?
> 
> `getDynamicLinker()` is implemented but never called in this patch -- is it 
> needed?
I don't know the rationale, I am just reusing the same principle as in 
Linux.{h,cpp}.

getDynamicLinker is used by Gnu.cpp's ConstructJob.



Comment at: lib/Driver/ToolChains/Hurd.h:35
+
+  std::vector ExtraOpts;
+

aaron.ballman wrote:
> This appears to be entirely unused.
Again, it is used by Gnu.cpp's ConstructJob


Repository:
  rC Clang

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

https://reviews.llvm.org/D54379



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

[PATCH] D54379: Add Hurd toolchain support to Clang

[Samuel Thibault via Phabricator via cfe-commits]

sthibaul updated this revision to Diff 175186.
sthibaul marked 4 inline comments as done.

Repository:
  rC Clang

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

https://reviews.llvm.org/D54379

Files:
  lib/Basic/Targets.cpp
  lib/Basic/Targets/OSTargets.h
  lib/Driver/CMakeLists.txt
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/Hurd.cpp
  lib/Driver/ToolChains/Hurd.h
  lib/Frontend/InitHeaderSearch.cpp
  test/Driver/Inputs/basic_hurd_tree/include/.keep
  test/Driver/Inputs/basic_hurd_tree/lib/i386-gnu/.keep
  test/Driver/Inputs/basic_hurd_tree/lib32/.keep
  test/Driver/Inputs/basic_hurd_tree/usr/include/i386-gnu/.keep
  test/Driver/Inputs/basic_hurd_tree/usr/lib/i386-gnu/.keep
  test/Driver/Inputs/basic_hurd_tree/usr/lib32/.keep
  test/Driver/hurd.c

Index: test/Driver/hurd.c
===
--- test/Driver/hurd.c
+++ test/Driver/hurd.c
@@ -0,0 +1,20 @@
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=i386-pc-gnu \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN:   | FileCheck --check-prefix=CHECK %s
+// CHECK-NOT: warning:
+// CHECK: "{{.*}}clang{{(.exe)?}}"
+// CHECK: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK: "-dynamic-linker" "/lib/ld.so"
+// CHECK: "crtbegin.o"
+// CHECK: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/lib"
+// CHECK: "-L[[SYSROOT]]/usr/lib"
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -260,6 +260,7 @@
 
   switch (os) {
   case llvm::Triple::Linux:
+  case llvm::Triple::Hurd:
   case llvm::Triple::Solaris:
 llvm_unreachable("Include management is handled in the driver.");
 
@@ -412,6 +413,7 @@
 
   switch (os) {
   case llvm::Triple::Linux:
+  case llvm::Triple::Hurd:
   case llvm::Triple::Solaris:
 llvm_unreachable("Include management is handled in the driver.");
 break;
@@ -460,6 +462,7 @@
 break; // Everything else continues to use this routine's logic.
 
   case llvm::Triple::Linux:
+  case llvm::Triple::Hurd:
   case llvm::Triple::Solaris:
 return;
 
Index: lib/Driver/ToolChains/Hurd.h
===
--- lib/Driver/ToolChains/Hurd.h
+++ lib/Driver/ToolChains/Hurd.h
@@ -0,0 +1,46 @@
+//===--- Hurd.h - Hurd ToolChain Implementations --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+
+#include "Gnu.h"
+#include "clang/Driver/ToolChain.h"
+
+namespace clang {
+namespace driver {
+namespace toolchains {
+
+class LLVM_LIBRARY_VISIBILITY Hurd : public Generic_ELF {
+public:
+  Hurd(const Driver &D, const llvm::Triple &Triple,
+   const llvm::opt::ArgList &Args);
+
+  bool HasNativeLLVMSupport() const override;
+
+  void
+  AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+llvm::opt::ArgStringList &CC1Args) const override;
+
+  virtual std::string computeSysRoot() const;
+
+  virtual std::string getDynamicLinker(const llvm::opt::ArgList &Args) const;
+
+  std::vector ExtraOpts;
+
+protected:
+  Tool *buildAssembler() const override;
+  Tool *buildLinker() const override;
+};
+
+} // end namespace toolchains
+} // end namespace driver
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
Index: lib/Driver/ToolChains/Hurd.cpp
===
--- lib/Driver/ToolChains/Hurd.cpp
+++ lib/Driver/ToolChains/Hurd.cpp
@@ -0,0 +1,169 @@
+//===--- Hurd.cpp - Hurd ToolChain Implementations *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Hurd.h"
+#include "CommonArgs.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Options.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem

[PATCH] D54379: Add Hurd toolchain support to Clang

[Samuel Thibault via Phabricator via cfe-commits]

sthibaul added a comment.

(there still needs to be work on the test part)


Repository:
  rC Clang

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

https://reviews.llvm.org/D54379



___
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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

https://reviews.llvm.org/rC347417 makes `constexpr string_view service = "HELLO 
WORD SERVICE"` (P0426) broken with libstdc++

  % cat a.cc
  constexpr bool __constant_string_p(const char *__s) {
while (__builtin_constant_p(*__s) && *__s)
  __s++;
return __builtin_constant_p(*__s);
  }
  
  constexpr bool n = __constant_string_p("a");



  % fclang++ -std=c++17 -fsyntax-only a.cc
  a.cc:1:16: error: constexpr function never produces a constant expression 
[-Winvalid-constexpr]
  constexpr bool __constant_string_p(const char *__s) {
 ^
  a.cc:2:10: note: subexpression not valid in a constant expression
while (__builtin_constant_p(*__s) && *__s)
   ^
  a.cc:7:16: error: constexpr variable 'n' must be initialized by a constant 
expression
  constexpr bool n = __constant_string_p("a");
 ^   
  a.cc:2:10: note: subexpression not valid in a constant expression
while (__builtin_constant_p(*__s) && *__s)
   ^
  a.cc:7:20: note: in call to '__constant_string_p(&"a"[0])'
  constexpr bool n = __constant_string_p("a");
 ^
  2 errors generated.

Fourth time should be a charm...


Repository:
  rC Clang

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

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] D54379: Add Hurd toolchain support to Clang

[Samuel Thibault via Phabricator via cfe-commits]

sthibaul updated this revision to Diff 175188.

Repository:
  rC Clang

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

https://reviews.llvm.org/D54379

Files:
  lib/Basic/Targets.cpp
  lib/Basic/Targets/OSTargets.h
  lib/Driver/CMakeLists.txt
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/Hurd.cpp
  lib/Driver/ToolChains/Hurd.h
  lib/Frontend/InitHeaderSearch.cpp
  test/Driver/Inputs/basic_hurd_tree/include/.keep
  test/Driver/Inputs/basic_hurd_tree/lib/i386-gnu/.keep
  test/Driver/Inputs/basic_hurd_tree/lib32/.keep
  test/Driver/Inputs/basic_hurd_tree/usr/include/i386-gnu/.keep
  test/Driver/Inputs/basic_hurd_tree/usr/lib/i386-gnu/.keep
  test/Driver/Inputs/basic_hurd_tree/usr/lib32/.keep
  test/Driver/hurd.c

Index: test/Driver/hurd.c
===
--- test/Driver/hurd.c
+++ test/Driver/hurd.c
@@ -0,0 +1,62 @@
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN:   | FileCheck --check-prefix=CHECK %s
+// CHECK-NOT: warning:
+// CHECK: "-cc1"
+// CHECK: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK: "-dynamic-linker" "/lib/ld.so"
+// CHECK: "crtbegin.o"
+// CHECK: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK: "-L[[SYSROOT]]/lib"
+// CHECK: "-L[[SYSROOT]]/usr/lib"
+
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu -static \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-STATIC %s
+// CHECK-STATIC-NOT: warning:
+// CHECK-STATIC: "-cc1"
+// CHECK-STATIC: "-static-define"
+// CHECK-STATIC: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-STATIC: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK-STATIC: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK-STATIC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-STATIC: "-static"
+// CHECK-STATIC: "crtbeginT.o"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK-STATIC: "-L[[SYSROOT]]/lib"
+// CHECK-STATIC: "-L[[SYSROOT]]/usr/lib"
+
+// RUN: %clang -no-canonical-prefixes %s -### 2>&1 \
+// RUN: --target=i386-pc-gnu -shared \
+// RUN: --sysroot=%S/Inputs/basic_hurd_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-SHARED %s
+// CHECK-SHARED-NOT: warning:
+// CHECK-SHARED: "-cc1"
+// CHECK-SHARED: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-SHARED: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/usr/include/i386-gnu"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/include"
+// CHECK-SHARED: "-internal-externc-isystem" "[[SYSROOT]]/usr/include"
+// CHECK-SHARED: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-SHARED: "crtbeginS.o"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib/i386-gnu"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib/../lib32"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib/i386-gnu"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib/../lib32"
+// CHECK-SHARED: "-L[[SYSROOT]]/lib"
+// CHECK-SHARED: "-L[[SYSROOT]]/usr/lib"
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -260,6 +260,7 @@
 
   switch (os) {
   case llvm::Triple::Linux:
+  case llvm::Triple::Hurd:
   case llvm::Triple::Solaris:
 llvm_unreachable("Include management is handled in the driver.");
 
@@ -412,6 +413,7 @@
 
   switch (os) {
   case llvm::Triple::Linux:
+  case llvm::Triple::Hurd:
   case llvm::Triple::Solaris:
 llvm_unreachable("Include management is handled in the driver.");
 break;
@@ -460,6 +462,7 @@
 break; // Everything else continues to use this routine's logic.
 
   case llvm::Triple::Linux:
+  case llvm::Triple::Hurd:
   case llvm::Triple::Solaris:
 return;
 
Index: lib/Driver/ToolChains/Hurd.h
===
--- lib/Driver/ToolChains/Hurd.h
+++ lib/Driver/ToolChains/Hurd.h
@@ -0,0 +1,46 @@
+//===--- Hurd.h - Hurd ToolChain Implementations --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure

[PATCH] D54379: Add Hurd toolchain support to Clang

[Samuel Thibault via Phabricator via cfe-commits]

sthibaul added a comment.

I have added static and shared library tests

> a short unit test with regards to creating the actual target instance)

I'm not sure what you mean? The tests create an actual binary for the target.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54379



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

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

[Alexander von Gluck IV via Phabricator via cfe-commits]

kallisti5 added a comment.

sorry, busy weekend. If you find the time feel free :-)

- _GLIBCXX_USE_FLOAT128 can (and should) go
- everything else is valid per the discussions here.

Otherwise i'll pick up in the next few days and add the requested context as 
well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53696



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

Doh! I'll take a look at it.


Repository:
  rC Clang

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

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] D54878: [clangd] NFC: Eliminate the unused variable warning.

[Fangrui Song via Phabricator via cfe-commits]

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



Comment at: clangd/AST.cpp:98
   // The name was empty, so present an anonymous entity.
-  if (auto *NS = llvm::dyn_cast(&ND))
+  if (isa(&ND))
 return "(anonymous namespace)";

`isa(ND)` also works.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54878



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

r347531 - A "constexpr" is evaluated in a constant context. Make sure this is reflected

[Bill Wendling via cfe-commits]

Author: void
Date: Sun Nov 25 18:10:53 2018
New Revision: 347531

URL: http://llvm.org/viewvc/llvm-project?rev=347531&view=rev
Log:
A "constexpr" is evaluated in a constant context. Make sure this is reflected
if a __builtin_constant_p() is a part of a constexpr.

Modified:
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp

Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=347531&r1=347530&r2=347531&view=diff
==
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Sun Nov 25 18:10:53 2018
@@ -10882,6 +10882,7 @@ bool Expr::EvaluateAsInitializer(APValue
   ? EvalInfo::EM_ConstantExpression
   : EvalInfo::EM_ConstantFold);
   InitInfo.setEvaluatingDecl(VD, Value);
+  InitInfo.InConstantContext = true;
 
   LValue LVal;
   LVal.set(VD);
@@ -11544,6 +11545,7 @@ bool Expr::isPotentialConstantExpr(const
 
   EvalInfo Info(FD->getASTContext(), Status,
 EvalInfo::EM_PotentialConstantExpression);
+  Info.InConstantContext = true;
 
   const CXXMethodDecl *MD = dyn_cast(FD);
   const CXXRecordDecl *RD = MD ? MD->getParent()->getCanonicalDecl() : nullptr;

Modified: cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp?rev=347531&r1=347530&r2=347531&view=diff
==
--- cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp (original)
+++ cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp Sun Nov 25 18:10:53 
2018
@@ -1122,3 +1122,11 @@ constexpr E e2 = E{0};
 static_assert(e2.x != e2.y, "");
 
 } // namespace IndirectFields
+
+constexpr bool __constant_string_p(const char *__s) {
+  while (__builtin_constant_p(*__s) && *__s)
+__s++;
+  return __builtin_constant_p(*__s);
+}
+
+constexpr bool n = __constant_string_p("a");


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

Fixed in r347531.


Repository:
  rC Clang

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

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] D54878: [clangd] NFC: Eliminate the unused variable warning.

[Henry Wong via Phabricator via cfe-commits]

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



Comment at: clangd/AST.cpp:98
   // The name was empty, so present an anonymous entity.
-  if (auto *NS = llvm::dyn_cast(&ND))
+  if (isa(&ND))
 return "(anonymous namespace)";

MaskRay wrote:
> `isa(ND)` also works.
Thanks for the tips! I will update it latter.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54878



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

[PATCH] D54792: add -march=cascadelake support in clang

[Craig Topper via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D54792



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

Re: [clang-tools-extra] r347498 - [clangd] Add support for hierarchical documentSymbol

[Mikael Holmén via cfe-commits]

Hi Ilya,

This patch doesn't compile for me with clang 3.6.0. I get:

../tools/clang/tools/extra/clangd/Protocol.cpp:474:10: error: no viable 
conversion from 'json::Object' to 'llvm::json::Value'
   return Result;
  ^~
../include/llvm/Support/JSON.h:291:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'const 
llvm::json::Value &' for 1st argument
   Value(const Value &M) { copyFrom(M); }
   ^
../include/llvm/Support/JSON.h:292:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'llvm::json::Value 
&&' for 1st argument
   Value(Value &&M) { moveFrom(std::move(M)); }
   ^
../include/llvm/Support/JSON.h:293:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 
'std::initializer_list' for 1st argument
   Value(std::initializer_list Elements);
   ^
../include/llvm/Support/JSON.h:294:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'json::Array &&' for 
1st argument
   Value(json::Array &&Elements) : Type(T_Array) {
   ^
../include/llvm/Support/JSON.h:299:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'json::Object &&' for 
1st argument
   Value(json::Object &&Properties) : Type(T_Object) {
   ^
../include/llvm/Support/JSON.h:305:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'std::string' (aka 
'basic_string') for 1st argument
   Value(std::string V) : Type(T_String) {
   ^
../include/llvm/Support/JSON.h:312:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'const 
llvm::SmallVectorImpl &' for 1st argument
   Value(const llvm::SmallVectorImpl &V)
   ^
../include/llvm/Support/JSON.h:314:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'const 
llvm::formatv_object_base &' for 1st argument
   Value(const llvm::formatv_object_base &V) : Value(V.str()){};
   ^
../include/llvm/Support/JSON.h:316:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'llvm::StringRef' for 
1st argument
   Value(StringRef V) : Type(T_StringRef) {
   ^
../include/llvm/Support/JSON.h:323:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'const char *' for 
1st argument
   Value(const char *V) : Value(StringRef(V)) {}
   ^
../include/llvm/Support/JSON.h:324:3: note: candidate constructor not 
viable: no known conversion from 'json::Object' to 'std::nullptr_t' (aka 
'nullptr_t') for 1st argument
   Value(std::nullptr_t) : Type(T_Null) {}
   ^
../include/llvm/Support/JSON.h:298:3: note: candidate template ignored: 
could not match 'vector >' against 'llvm::json::Object'
   Value(const std::vector &C) : Value(json::Array(C)) {}
   ^
../include/llvm/Support/JSON.h:303:3: note: candidate template ignored: 
could not match 'map, type-parameter-0-0, 
std::less >, allocator, type-parameter-0-0> > >' against 
'llvm::json::Object'
   Value(const std::map &C) : Value(json::Object(C)) {}
   ^
../include/llvm/Support/JSON.h:329:42: note: candidate template ignored: 
disabled by 'enable_if' [with T = llvm::json::Object]
   typename = typename std::enable_if::value>::type,
  ^
../include/llvm/Support/JSON.h:337:42: note: candidate template ignored: 
disabled by 'enable_if' [with T = llvm::json::Object]
   typename = typename std::enable_if::value>::type,
  ^
../include/llvm/Support/JSON.h:345:41: note: candidate template ignored: 
disabled by 'enable_if' [with T = llvm::json::Object]
 typename 
std::enable_if::value>::type,
 ^
../include/llvm/Support/JSON.h:355:3: note: candidate template ignored: 
substitution failure [with T = llvm::json::Object]: no matching function 
for call to 'toJSON'
   Value(const T &V) : Value(toJSON(V)) {}
   ^
1 error generated.
ninja: build stopped: subcommand failed.
system(/proj/flexasic/app/ninja/1.8.2/SLED11-64/bin/ninja -j1 -C 
build-all  all) failed: child exited with value 1


A couple of the build bots fail the same way, see e.g:

 
http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/5512/steps/build%20stage%201/logs/stdio


-


This patch also causes a couple of new warnings:

../tools/clang/tools/extra/clangd/AST.cpp:98:13: error: unused variable 
'NS' [-Werror,-Wunused-variable]
   if (auto *NS = llvm::dyn_cast(&ND))
 ^
../tools/clang/tools/extra/clangd/AST.cpp:102:13: error: unused variable 
'En' [-Werror,-Wunused-variable]
   if (auto *En = llvm::dyn_cast(&ND))
 ^
2 errors generated.

/Mikael

On 11/23/18 4:21 PM, Ilya Biryukov via cfe-commits wrote:
> Author: ibiryukov
> Date: Fri Nov 23 07:21:19 2018
> New Revision: 347498
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=347498&view=rev
> Log:
> [clangd] Add support for hierarchical documentSym

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:80
+// used with the same version of generated operators.
+RecTy = Context.getAddrSpaceQualType(RecTy, LangAS::opencl_generic);
+

I would suggest taking this opportunity to set up the AST to support declaring 
methods in an arbitrary address space, so that you can just ask a 
`CXXMethodDecl` what address space it's in.  You don't have to actually add 
language support for that — OpenCL C++ would simply change the it to the 
generic address space instead of the default — but I think that's the right 
technical approach for implementing this, as opposed to adding a bunch of 
OpenCL C++ -specific logic all over the compiler that just hardcodes a 
different address space.



Comment at: lib/CodeGen/CGCall.cpp:4035
+  V = Builder.CreatePointerBitCastOrAddrSpaceCast(V, DestTy);
+}
 

Always use the `performAddrSpaceConversion` target hook if there's a semantic 
address-space conversion required.  But really, this doesn't seem like the 
right place to be doing this; it ought to happen higher up when we're adding 
the `this` argument to the call, either explicitly in IRGen or implicitly by 
expecting the object expression to already yield a value in the right address 
space.

I could definitely believe that we don't currently create `CastExpr`s for 
simple qualification conversions of the object argument of a C++ method call, 
since (ignoring these address-space conversions) they're always trivial.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54862



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