[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

Apologies once again for the delayed response. I reviewed some today and will 
resume reviewing on Monday.

In addition to the inline suggestions:

`clang/docs/ReleaseNotes.rst` will need to be updated to reflect that the core 
language changes for P1467R9 have been implemented for `std::float16_t` and 
`std::bfloat16_t`.

Given the confusing array of 16-bit floating-point types, how about modifying 
`clang/include/clang/AST/BuiltinTypes.def` to be more explicit regarding which 
is which?

  // 'half' in OpenCL, '__fp16' in ARM NEON.
  FLOATING_TYPE(Half, HalfTy)
  ...
  // '_Float16', 'std::float16_t'
  FLOATING_TYPE(Float16, HalfTy)
  
  // '__bf16', 'std::bfloat16_t'
  FLOATING_TYPE(BFloat16, BFloat16Ty)

Hmm, having pasted the above, I now noticed that the `Half` and `Float16` types 
share the `HalfTy` singleton. Unless I'm mistaken, the changes being made will 
cause divergent behavior. Do these now need to be split?




Comment at: clang/include/clang/AST/ASTContext.h:56
+// Conversion ranks introduced in C++23 6.8.6p2 [conv.rank]
+enum FloatingRankCompareResult {
+  FRCR_Unordered,

Naming suggestion: `FloatConvRankCompareResult`.



Comment at: clang/include/clang/AST/ASTContext.h:1119
+  CanQualType BFloat16Ty; // [C++23 6.8.3p5][basic.extended.fp]
+  CanQualType Float16Ty; // C11 extension ISO/IEC TS 18661-3 and [C++23 
6.8.3p5][basic.extended.fp]
   CanQualType VoidPtrTy, NullPtrTy;

I think the comment should reference http://eel.is/c++draft/basic.extended.fp#1 
for `std::float16_t`. p5 is for `std::bfloat16_t`.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8931
 >;
+def err_cxx23_invalid_implicit_floating_point_cast : Error<"floating point 
cast results in loss of precision">;
 def err_typecheck_expect_scalar_operand : Error<

The diagnostic text here looks more like the text of a warning. Since this is 
an error, I think it makes more sense to model the text on other similar errors 
and use "narrowing" or "implicit cast" terminology. For examples, see 
`err_spaceship_argument_narrowing` and `err_impcast_complex_scalar`

 Additionally, it would be helpful to include the relevant types in the message.

Finally, line length should be kept to 80 characters or less per 
https://llvm.org/docs/CodingStandards.html#source-code-width.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:779-788
+FloatingRankCompareResult order = Ctx.getFloatingTypeOrder(LTy, RTy);
+if ((order == FRCR_Greater) || (order == FRCR_Equal_Greater_Subrank)) {
   RHS = (*doCast)(Solver, RHS, LTy, LBitWidth, RTy, RBitWidth);
   RTy = LTy;
-} else if (order == 0) {
+} else if ((order == FRCR_Equal) || (order == FRCR_Equal_Lesser_Subrank)) {
   LHS = (*doCast)(Solver, LHS, RTy, RBitWidth, LTy, LBitWidth);
   LTy = RTy;

This looks like a pre-existing bug since, for standard floating-point types, 
narrowing implicit conversions are allowed. I think the intent was to add a 
cast on which ever side is needed, but the existing code instead adds a cast on 
the RHS when the LHS has a higher rank, adds a cast on the LHS when the LHS and 
RHS have the same rank, and wanders into UB when the RHS has a higher rank.

The incorrect comparison goes back to when the code was introduced in commit 
08f943c5630d8ee31d6a93227171d2f05db59e62.

The introduction of unordered conversion ranks suggests additional changes are 
needed here, but it isn't clear to me what the right changes are. I added a 
suggested edit that adds an arbitrary cast (which probably suffices for the 
purposes of static analysis). Alternatively, an assert could be added for the 
unordered and equal cases.



Comment at: clang/lib/Sema/SemaChecking.cpp:10451
   return ICE->getCastKind() == CK_FloatingCast &&
- S.Context.getFloatingTypeOrder(From, To) < 0;
+ S.Context.getFloatingTypeOrder(From, To) == FRCR_Lesser;
 }

codemzs wrote:
> tahonermann wrote:
> > I'm not sure this is right. If I understand correctly, the C++23 extended 
> > FP types don't participate in argument promotions. Perhaps they should be 
> > excluded here.
> Rules for 
> [promotion](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1467r9.html#promotion)
>  are unchanged as per the proposal. This is just using the new enumeration to 
> represent a smaller conversion rank. 
I think this still produces an incorrect result in some cases though. According 
to http://eel.is/c++draft/conv.fpprom, the only floating-point promotion is 
from `float` to `double`.

Ah, I think the prior code was incorrect, but non-symptomatic because it is 
only currently used in one place (in `CheckPrintfHandler::checkFormatExpr()`) 
and that code only cares about the integer cases. I would prefer that we fix 
this rather than extend the issue into

[PATCH] D156565: Diagnose use of VLAs in C++ by default

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:147-149
+def ext_vla_cxx_static_assert : ExtWarn<
+  "variable length arrays in C++ are a Clang extension; did you mean to use "
+  "'static_assert'?">, InGroup;

aaron.ballman wrote:
> tahonermann wrote:
> > I find the "did you mean to use 'static_assert'" phrasing awkward here 
> > since it is highly unlikely that someone intended to write 
> > `static_assert(x)` and instead wrote `int my_assert[x ? 1 : -1]`. Perhaps 
> > something like this?
> > 
> > "variable length arrays in C++ are a Clang extension; 'static_assert' can 
> > be used in this case".
> Eh, I'm on the fence. We're consistent about asking users "did you mean X?" 
> in our diagnostics and this follows the same pattern. "Did you mean to use X" 
> is not "is this a typo for X?" but "were you aware you could do X instead?" 
> So yeah, the wording is a bit awkward, but it's consistent with other 
> diagnostics and not really wrong either. Do you have strong feelings?
No, I don't have strong feelings on this.

I did recognize use of the widely used "did you mean" pattern, but my 
impression with those has been that they are generally employed in situations 
where similar syntax is involved (e.g., misspelled name, `.` vs `->`, `return` 
vs `co_return`, etc...) or where there might be some missing syntax (e.g., `()` 
following a function name or a missing `;`); cases where the programmer is 
likely to response "oh, yes I did!". This feels different since it is 
suggesting use of a distinct feature.


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

https://reviews.llvm.org/D156565

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

[PATCH] D156565: Diagnose use of VLAs in C++ by default

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:147-149
+def ext_vla_cxx_static_assert : ExtWarn<
+  "variable length arrays in C++ are a Clang extension; did you mean to use "
+  "'static_assert'?">, InGroup;

I find the "did you mean to use 'static_assert'" phrasing awkward here since it 
is highly unlikely that someone intended to write `static_assert(x)` and 
instead wrote `int my_assert[x ? 1 : -1]`. Perhaps something like this?

"variable length arrays in C++ are a Clang extension; 'static_assert' can be 
used in this case".



Comment at: clang/test/SemaCXX/vla-ext-diag.cpp:34
+   off-note {{function parameter 'n' with 
unknown value cannot be used in a constant expression}}
+}

Perhaps add a test where the conditional expression is parenthesized.
  int array4[(n ? 1 : -1)]; 

Adding tests for one side being of size 0 might be useful to demonstrate that 
those are not intended to trigger the "use 'static_assert'" diagnostic. I know 
some compilers don't diagnose on a size of 0 and so the static assert idiom 
generally requires a negative number.
  int array5[n ? 1 : 0]; // ok?


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

https://reviews.llvm.org/D156565

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

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

@codemzs, I'm sorry for the very long delay following up on this review. I've 
been struggling to keep up, but expect to be able to devote some time to this 
next week. I'm committed to helping to ensure we land this before Phabricator 
stops accepting new diffs (proposed for November 15th).


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

https://reviews.llvm.org/D149573

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

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

[Tom Honermann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG256a0b298c68: [clang] Correct source locations for 
instantiations of function templates. (authored by tahonermann).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64087

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
  clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
  clang/test/SemaCXX/member-init.cpp
  clang/test/SemaTemplate/virtual-member-functions.cpp

Index: clang/test/SemaTemplate/virtual-member-functions.cpp
===
--- clang/test/SemaTemplate/virtual-member-functions.cpp
+++ clang/test/SemaTemplate/virtual-member-functions.cpp
@@ -7,10 +7,10 @@
 
 namespace PR5557 {
 template  struct A {
-  A(); // expected-note{{instantiation}}
+  A();
   virtual int a(T x);
 };
-template A::A() {}
+template A::A() {} // expected-note{{instantiation}}
 
 template int A::a(T x) { 
   return *x; // expected-error{{requires pointer operand}}
@@ -33,10 +33,10 @@
 namespace PR5557_dtor {
 template  struct A {
   A(); // Don't have an implicit constructor.
-  ~A(); // expected-note{{instantiation}}
+  ~A();
   virtual int a(T x);
 };
-template A::~A() {}
+template A::~A() {} // expected-note{{instantiation}}
 
 template int A::a(T x) { 
   return *x; // expected-error{{requires pointer operand}}
Index: clang/test/SemaCXX/member-init.cpp
===
--- clang/test/SemaCXX/member-init.cpp
+++ clang/test/SemaCXX/member-init.cpp
@@ -164,11 +164,11 @@
 
 namespace explicit_instantiation {
 template struct X {
-  X(); // expected-note {{in instantiation of default member initializer 'explicit_instantiation::X::n' requested here}}
+  X();
   int n = T::error; // expected-error {{type 'float' cannot be used prior to '::' because it has no members}}
 };
 template struct X; // ok
-template X::X() {}
+template X::X() {} // expected-note {{in instantiation of default member initializer 'explicit_instantiation::X::n' requested here}}
 template struct X; // expected-note {{in instantiation of member function 'explicit_instantiation::X::X' requested here}}
 }
 
@@ -197,3 +197,15 @@
 }
 template void foo(int);
 }
+
+namespace GH26057 {
+template
+struct S {
+  S();
+  int dm = T::error; // expected-error {{type 'int' cannot be used prior to '::' because it has no members}}
+};
+template
+S::S() = default; // expected-note {{in instantiation of default member initializer 'GH26057::S::dm' requested here}} \
+ // expected-note {{in evaluation of exception specification for 'GH26057::S::S' needed here}}
+template struct S; // expected-note {{in instantiation of member function 'GH26057::S::S' requested here}}
+}
Index: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
===
--- clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
+++ clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
@@ -102,14 +102,14 @@
 
 namespace forward_declare_consteval{
 template 
-constexpr int f(T t);  // expected-note {{'f' defined here}}
+constexpr int f(T t);
 
 auto a = &f;
 auto b = &f; // expected-error {{immediate function 'f' used before it is defined}} \
   // expected-note {{in instantiation of function template specialization}}
 
 template 
-constexpr int f(T t) {
+constexpr int f(T t) { // expected-note {{'f' defined here}}
 return id(t); // expected-note {{'f' is an immediate function because its body contains a call to a consteval function 'id' and that call is not a constant expression}}
 }
 }
Index: clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
===
--- clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
+++ clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
@@ -209,10 +209,10 @@
 
 namespace p2085_2 {
 template  struct S6 {
-  // expected-error@+2{{found 'const int &'}}
-  // expected-error@+1{{found 'const float &'}}
   bool operator==(T const &) const;
 };
+// expected-error@+2{{found 'const int &'}}
+// expected-error@+1{{found 'const float &'}}
 template  bool S6::operator==(T const &) const = default;
 
 template struct S6; // expected-note{{S6::operator==' requested}}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4990,8 +4990,10 @@
   // unimported module.
   Function->setVisibleDespiteOwningModule();
 
-  // Copy the inner loc start from the pattern.
+  // Copy the source locations from the pattern.
+  Function->se

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

[Tom Honermann via Phabricator via cfe-commits]

tahonermann updated this revision to Diff 556970.
tahonermann added a comment.

Addressed review feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64087

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
  clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
  clang/test/SemaCXX/member-init.cpp
  clang/test/SemaTemplate/virtual-member-functions.cpp

Index: clang/test/SemaTemplate/virtual-member-functions.cpp
===
--- clang/test/SemaTemplate/virtual-member-functions.cpp
+++ clang/test/SemaTemplate/virtual-member-functions.cpp
@@ -7,10 +7,10 @@
 
 namespace PR5557 {
 template  struct A {
-  A(); // expected-note{{instantiation}}
+  A();
   virtual int a(T x);
 };
-template A::A() {}
+template A::A() {} // expected-note{{instantiation}}
 
 template int A::a(T x) { 
   return *x; // expected-error{{requires pointer operand}}
@@ -33,10 +33,10 @@
 namespace PR5557_dtor {
 template  struct A {
   A(); // Don't have an implicit constructor.
-  ~A(); // expected-note{{instantiation}}
+  ~A();
   virtual int a(T x);
 };
-template A::~A() {}
+template A::~A() {} // expected-note{{instantiation}}
 
 template int A::a(T x) { 
   return *x; // expected-error{{requires pointer operand}}
Index: clang/test/SemaCXX/member-init.cpp
===
--- clang/test/SemaCXX/member-init.cpp
+++ clang/test/SemaCXX/member-init.cpp
@@ -164,11 +164,11 @@
 
 namespace explicit_instantiation {
 template struct X {
-  X(); // expected-note {{in instantiation of default member initializer 'explicit_instantiation::X::n' requested here}}
+  X();
   int n = T::error; // expected-error {{type 'float' cannot be used prior to '::' because it has no members}}
 };
 template struct X; // ok
-template X::X() {}
+template X::X() {} // expected-note {{in instantiation of default member initializer 'explicit_instantiation::X::n' requested here}}
 template struct X; // expected-note {{in instantiation of member function 'explicit_instantiation::X::X' requested here}}
 }
 
@@ -197,3 +197,15 @@
 }
 template void foo(int);
 }
+
+namespace GH26057 {
+template
+struct S {
+  S();
+  int dm = T::error; // expected-error {{type 'int' cannot be used prior to '::' because it has no members}}
+};
+template
+S::S() = default; // expected-note {{in instantiation of default member initializer 'GH26057::S::dm' requested here}} \
+ // expected-note {{in evaluation of exception specification for 'GH26057::S::S' needed here}}
+template struct S; // expected-note {{in instantiation of member function 'GH26057::S::S' requested here}}
+}
Index: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
===
--- clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
+++ clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
@@ -102,14 +102,14 @@
 
 namespace forward_declare_consteval{
 template 
-constexpr int f(T t);  // expected-note {{'f' defined here}}
+constexpr int f(T t);
 
 auto a = &f;
 auto b = &f; // expected-error {{immediate function 'f' used before it is defined}} \
   // expected-note {{in instantiation of function template specialization}}
 
 template 
-constexpr int f(T t) {
+constexpr int f(T t) { // expected-note {{'f' defined here}}
 return id(t); // expected-note {{'f' is an immediate function because its body contains a call to a consteval function 'id' and that call is not a constant expression}}
 }
 }
Index: clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
===
--- clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
+++ clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
@@ -209,10 +209,10 @@
 
 namespace p2085_2 {
 template  struct S6 {
-  // expected-error@+2{{found 'const int &'}}
-  // expected-error@+1{{found 'const float &'}}
   bool operator==(T const &) const;
 };
+// expected-error@+2{{found 'const int &'}}
+// expected-error@+1{{found 'const float &'}}
 template  bool S6::operator==(T const &) const = default;
 
 template struct S6; // expected-note{{S6::operator==' requested}}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4990,8 +4990,10 @@
   // unimported module.
   Function->setVisibleDespiteOwningModule();
 
-  // Copy the inner loc start from the pattern.
+  // Copy the source locations from the pattern.
+  Function->setLocation(PatternDecl->getLocation());
   Function->setInnerLocStart(PatternDecl->getInnerLocStart());
+  Function->setRangeEnd(PatternDecl->getEndLoc());
 
 

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

@cor3ntin, any concerns or suggestions per my recent updates? I'll plan to land 
this in the next couple of days otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64087

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

[PATCH] D159474: [NFC][CLANG] Fix static analyzer bugs about unnecessary object copies with auto

[Tom Honermann via Phabricator via cfe-commits]

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

Thanks, @Manna, these changes look good to me!




Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:175
 SmallVector Bases;
-for (const auto BaseSpecifier : Decl->bases()) {
+for (const auto &BaseSpecifier : Decl->bases()) {
   // skip classes not inherited as public

I agree that this looks like a good change. `CXXBaseSpecifier` contains a 
`SourceRange`, a `SourceLocation`, a `unsigned` (used as a bit-field, and a 
`TypeSourceInfo*`. Those are all individually cheap to copy, but together are 
large enough to justify use of a reference.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:2256
 
-  for (auto [VD, Ignore] : FixItsForVariable) {
+  for (const auto &[VD, Ignore] : FixItsForVariable) {
 VarGrpRef Grp = VarGrpMgr.getGroupOfVar(VD);

This looks good too. The returned map value has `const VarDecl*` as a key and 
the element type is `FixItList` (aka `SmallVector`) . `FixItHint` 
contains two `CharSourceRange` objects, a `std::string`, and a `bool`, so we 
definitely don't want to be copying that; especially since the value isn't even 
wanted here!



Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:903
   Symbols.emplace_back(std::move(*Class));
-  for (const auto Base : Record.Bases)
+  for (const auto &Base : Record.Bases)
 serializeRelationship(RelationshipKind::InheritsFrom, Record, Base);

Another case of `CXXBaseSpecifier` which, per earlier comments, is large enough 
to justify use of a reference.


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

https://reviews.llvm.org/D159474

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:304
+static_assert('\u{9}' == (char)1, ""); // expected-error {{failed}} \
+   // expected-note {{evaluates to ''\t' 
(0x09, 9) == '' (0x01, 1)'}}
+static_assert((char8_t)-128 == (char8_t)-123, ""); // expected-error 
{{failed}} \

cor3ntin wrote:
> tahonermann wrote:
> > cor3ntin wrote:
> > > tahonermann wrote:
> > > > Is the expected note up to date? I don't see code that would generate 
> > > > the `` output. Am I just missing it? Since U+0001 is a valid, 
> > > > though non-printable, character, I would expect more `'\u0001'`.
> > > See elsewhere in the discussion. this formating is pre existing and 
> > > managed at the DiagnosticEngine level (pushEscapedString). the reason 
> > > it's not `\u0001` is 1/ to avoid  reusing c++ syntactic elements for 
> > > something that comes from diagnostics and is not represented as an 
> > > escaped sequence in source 2/ `\u00011` is unreadable, and `\U1` 
> > > is also not helpful :)
> > > 
> > Thanks for the explanation. I'm not sure that I agree with the rationale 
> > for (1) though. We're already putting the value in single quotes and 
> > representing some values with escapes in many of these cases when the value 
> > isn't produced by an escape sequence (or even a character/string literal); 
> > why exclude `\u`? I agree with the rationale for (2); we could use 
> > `'\u{1}'` in that case.
> FYI afaik the notation in clang predates the existence of \u{} by a few 
> years, and follow Unicode notation 
> (https://unicode.org/mail-arch/unicode-ml/y2005-m11/0060.html).
> Oldest instance seems to be 
> https://github.com/llvm/llvm-project/commit/77091b167fd959e1ee0c4dad4ec44de43b6c95db
>  - i followed suite when reworking the generic escaping mechanism all string 
> fed to diagnostics go through.
> 
> I don't care about changing the syntax, but i do hope we are consistent. 
> Ultimately what we are trying to do is to designate a unicode codepoint and 
> whether we do it through C++ syntax or not probably does not matter much as 
> long as it's clear, delimited and consistent!
I think the substitution of `` by the diagnostic engine itself is 
perfectly fine and good; particularly when it has no context to suggest a 
different presentation. In this particular case, where the character is being 
presented using C++ syntax as a character literal, I would prefer that C++ 
syntax be used consistently.

From an implementation standpoint, I'm suggesting that 
`WriteCharValueForDiagnostic()` be modified such that, if 
`escapeCStyle()` returns an empty string, that the 
character be presented in `'\u{}'` form if the character is one that would 
otherwise be substituted by the diagnostic engine (e.g., if `isPrintable()` is 
false). Note that this would be restricted to `char` values <= 0x7F; larger 
values could still be passed through as invalid code units that the diagnostic 
engine would then render as, e.g., `''`.



Comment at: clang/test/SemaCXX/static-assert.cpp:287
+  static_assert((char16_t)L'ゆ' == L"C̵̭̯̠̎͌ͅť̺"[1], ""); // expected-error 
{{failed}} \
+  // expected-note {{evaluates 
to ''ゆ' (0x3086) == '̵' (0x335)'}}
+  static_assert(L"＼／"[1] == u'\xFFFD', ""); // expected-error {{failed}} \

cor3ntin wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > cor3ntin wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > The C++23 escaped string formatting facility would not generate a 
> > > > > trailing combining character like this. I recommend following suit.
> > > > > 
> > > > > Info on U+0335: 
> > > > > https://util.unicode.org/UnicodeJsps/character.jsp?a=0335
> > > > > 
> > > > This is way outside the scope of the patch. The diagnostic output 
> > > > facility has no understanding of combining characters or graphemes and 
> > > > do not attempt to match std::print. It probably would be an improvement 
> > > > but this patch is not trying to modify how all diagnostics are printed. 
> > > > (all of that logic is in Diagnostic.cpp)
> > > This patch is pushing the envelope of what appears in diagnostics. One 
> > > can also argue that someone writing
> > > ```
> > > static_assert(false, "\u0301");
> > > ```
> > > gets what they deserve, but that case does not have a big problem anyway 
> > > (because the provided message text appears after `: `).
> > > 
> > > This patch increases the exposure of the diagnostic output facility to 
> > > input that it does not handle well. I disagree that it is outside the 
> > > scope of this patch to insist that it does not generate such inputs to 
> > > the diagnostic output facility (even if a possible solution is to modify 
> > > the diagnostic output facility first).
> > @cor3ntin, do you have status quo examples for how grapheme-extending 
> > char

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

[Tom Honermann via Phabricator via cfe-commits]

tahonermann updated this revision to Diff 556604.
tahonermann added a comment.

Moved the added release note to the correct section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64087

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
  clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
  clang/test/SemaCXX/member-init.cpp
  clang/test/SemaTemplate/virtual-member-functions.cpp

Index: clang/test/SemaTemplate/virtual-member-functions.cpp
===
--- clang/test/SemaTemplate/virtual-member-functions.cpp
+++ clang/test/SemaTemplate/virtual-member-functions.cpp
@@ -7,10 +7,10 @@
 
 namespace PR5557 {
 template  struct A {
-  A(); // expected-note{{instantiation}}
+  A();
   virtual int a(T x);
 };
-template A::A() {}
+template A::A() {} // expected-note{{instantiation}}
 
 template int A::a(T x) { 
   return *x; // expected-error{{requires pointer operand}}
@@ -33,10 +33,10 @@
 namespace PR5557_dtor {
 template  struct A {
   A(); // Don't have an implicit constructor.
-  ~A(); // expected-note{{instantiation}}
+  ~A();
   virtual int a(T x);
 };
-template A::~A() {}
+template A::~A() {} // expected-note{{instantiation}}
 
 template int A::a(T x) { 
   return *x; // expected-error{{requires pointer operand}}
Index: clang/test/SemaCXX/member-init.cpp
===
--- clang/test/SemaCXX/member-init.cpp
+++ clang/test/SemaCXX/member-init.cpp
@@ -164,11 +164,11 @@
 
 namespace explicit_instantiation {
 template struct X {
-  X(); // expected-note {{in instantiation of default member initializer 'explicit_instantiation::X::n' requested here}}
+  X();
   int n = T::error; // expected-error {{type 'float' cannot be used prior to '::' because it has no members}}
 };
 template struct X; // ok
-template X::X() {}
+template X::X() {} // expected-note {{in instantiation of default member initializer 'explicit_instantiation::X::n' requested here}}
 template struct X; // expected-note {{in instantiation of member function 'explicit_instantiation::X::X' requested here}}
 }
 
@@ -197,3 +197,15 @@
 }
 template void foo(int);
 }
+
+namespace GH26057 {
+template
+struct S {
+  S();
+  int dm = T::error; // expected-error {{type 'int' cannot be used prior to '::' because it has no members}}
+};
+template
+S::S() = default; // expected-note {{in instantiation of default member initializer 'GH26057::S::dm' requested here}} \
+ // expected-note {{in evaluation of exception specification for 'GH26057::S::S' needed here}}
+template struct S; // expected-note {{in instantiation of member function 'GH26057::S::S' requested here}}
+}
Index: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
===
--- clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
+++ clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
@@ -102,14 +102,14 @@
 
 namespace forward_declare_consteval{
 template 
-constexpr int f(T t);  // expected-note {{'f' defined here}}
+constexpr int f(T t);
 
 auto a = &f;
 auto b = &f; // expected-error {{immediate function 'f' used before it is defined}} \
   // expected-note {{in instantiation of function template specialization}}
 
 template 
-constexpr int f(T t) {
+constexpr int f(T t) { // expected-note {{'f' defined here}}
 return id(t); // expected-note {{'f' is an immediate function because its body contains a call to a consteval function 'id' and that call is not a constant expression}}
 }
 }
Index: clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
===
--- clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
+++ clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
@@ -209,10 +209,10 @@
 
 namespace p2085_2 {
 template  struct S6 {
-  // expected-error@+2{{found 'const int &'}}
-  // expected-error@+1{{found 'const float &'}}
   bool operator==(T const &) const;
 };
+// expected-error@+2{{found 'const int &'}}
+// expected-error@+1{{found 'const float &'}}
 template  bool S6::operator==(T const &) const = default;
 
 template struct S6; // expected-note{{S6::operator==' requested}}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4990,8 +4990,10 @@
   // unimported module.
   Function->setVisibleDespiteOwningModule();
 
-  // Copy the inner loc start from the pattern.
+  // Copy source locations from the pattern.
+  Function->setLocation(PatternDecl->getLocation());
   Function->setInnerLocStart(PatternDecl->getInnerLocStart());
+  Function->setRangeEnd(PatternD

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

@Endill, thank you for the reminder about this old patch!

@cor3ntin, I added an additional test and updated a few other recently 
(relative to the original patch!) added tests. Would you be so kind as to give 
this another quick review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64087

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

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

[Tom Honermann via Phabricator via cfe-commits]

tahonermann updated this revision to Diff 556592.
tahonermann edited the summary of this revision.
tahonermann added a comment.

Rebased patch uploaded. This retains the same code change, but includes 
additional test updates for tests added since the first patch was submitted, as 
well as an additional patch to exercise a test case from 
https://github.com/llvm/llvm-project/issues/26057.

The changes made effectively complete changes originally made in commit 
12dcbf3eaa6d2c8b9ee814ddb8bf23bef644bfaf (Fixed implicit instantiations source 
range.) 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64087

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
  clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
  clang/test/SemaCXX/member-init.cpp
  clang/test/SemaTemplate/virtual-member-functions.cpp

Index: clang/test/SemaTemplate/virtual-member-functions.cpp
===
--- clang/test/SemaTemplate/virtual-member-functions.cpp
+++ clang/test/SemaTemplate/virtual-member-functions.cpp
@@ -7,10 +7,10 @@
 
 namespace PR5557 {
 template  struct A {
-  A(); // expected-note{{instantiation}}
+  A();
   virtual int a(T x);
 };
-template A::A() {}
+template A::A() {} // expected-note{{instantiation}}
 
 template int A::a(T x) { 
   return *x; // expected-error{{requires pointer operand}}
@@ -33,10 +33,10 @@
 namespace PR5557_dtor {
 template  struct A {
   A(); // Don't have an implicit constructor.
-  ~A(); // expected-note{{instantiation}}
+  ~A();
   virtual int a(T x);
 };
-template A::~A() {}
+template A::~A() {} // expected-note{{instantiation}}
 
 template int A::a(T x) { 
   return *x; // expected-error{{requires pointer operand}}
Index: clang/test/SemaCXX/member-init.cpp
===
--- clang/test/SemaCXX/member-init.cpp
+++ clang/test/SemaCXX/member-init.cpp
@@ -164,11 +164,11 @@
 
 namespace explicit_instantiation {
 template struct X {
-  X(); // expected-note {{in instantiation of default member initializer 'explicit_instantiation::X::n' requested here}}
+  X();
   int n = T::error; // expected-error {{type 'float' cannot be used prior to '::' because it has no members}}
 };
 template struct X; // ok
-template X::X() {}
+template X::X() {} // expected-note {{in instantiation of default member initializer 'explicit_instantiation::X::n' requested here}}
 template struct X; // expected-note {{in instantiation of member function 'explicit_instantiation::X::X' requested here}}
 }
 
@@ -197,3 +197,15 @@
 }
 template void foo(int);
 }
+
+namespace GH26057 {
+template
+struct S {
+  S();
+  int dm = T::error; // expected-error {{type 'int' cannot be used prior to '::' because it has no members}}
+};
+template
+S::S() = default; // expected-note {{in instantiation of default member initializer 'GH26057::S::dm' requested here}} \
+ // expected-note {{in evaluation of exception specification for 'GH26057::S::S' needed here}}
+template struct S; // expected-note {{in instantiation of member function 'GH26057::S::S' requested here}}
+}
Index: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
===
--- clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
+++ clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
@@ -102,14 +102,14 @@
 
 namespace forward_declare_consteval{
 template 
-constexpr int f(T t);  // expected-note {{'f' defined here}}
+constexpr int f(T t);
 
 auto a = &f;
 auto b = &f; // expected-error {{immediate function 'f' used before it is defined}} \
   // expected-note {{in instantiation of function template specialization}}
 
 template 
-constexpr int f(T t) {
+constexpr int f(T t) { // expected-note {{'f' defined here}}
 return id(t); // expected-note {{'f' is an immediate function because its body contains a call to a consteval function 'id' and that call is not a constant expression}}
 }
 }
Index: clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
===
--- clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
+++ clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
@@ -209,10 +209,10 @@
 
 namespace p2085_2 {
 template  struct S6 {
-  // expected-error@+2{{found 'const int &'}}
-  // expected-error@+1{{found 'const float &'}}
   bool operator==(T const &) const;
 };
+// expected-error@+2{{found 'const int &'}}
+// expected-error@+1{{found 'const float &'}}
 template  bool S6::operator==(T const &) const = default;
 
 template struct S6; // expected-note{{S6::operator==' requested}}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:304
+static_assert('\u{9}' == (char)1, ""); // expected-error {{failed}} \
+   // expected-note {{evaluates to ''\t' 
(0x09, 9) == '' (0x01, 1)'}}
+static_assert((char8_t)-128 == (char8_t)-123, ""); // expected-error 
{{failed}} \

cor3ntin wrote:
> tahonermann wrote:
> > Is the expected note up to date? I don't see code that would generate the 
> > `` output. Am I just missing it? Since U+0001 is a valid, though 
> > non-printable, character, I would expect more `'\u0001'`.
> See elsewhere in the discussion. this formating is pre existing and managed 
> at the DiagnosticEngine level (pushEscapedString). the reason it's not 
> `\u0001` is 1/ to avoid  reusing c++ syntactic elements for something that 
> comes from diagnostics and is not represented as an escaped sequence in 
> source 2/ `\u00011` is unreadable, and `\U1` is also not helpful :)
> 
Thanks for the explanation. I'm not sure that I agree with the rationale for 
(1) though. We're already putting the value in single quotes and representing 
some values with escapes in many of these cases when the value isn't produced 
by an escape sequence (or even a character/string literal); why exclude 
`\u`? I agree with the rationale for (2); we could use `'\u{1}'` in that 
case.


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

https://reviews.llvm.org/D155610

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

[PATCH] D159345: [Clang] Handle non-ASCII after line splicing

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

Even better than I asked for. I held back on suggesting the change of `Tok` to 
`Result` to match `tryConsumeIdentifierUCN()`, but you made that change anyway! 
You must have read my mind! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159345

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

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:304
+static_assert('\u{9}' == (char)1, ""); // expected-error {{failed}} \
+   // expected-note {{evaluates to ''\t' 
(0x09, 9) == '' (0x01, 1)'}}
+static_assert((char8_t)-128 == (char8_t)-123, ""); // expected-error 
{{failed}} \

Is the expected note up to date? I don't see code that would generate the 
`` output. Am I just missing it? Since U+0001 is a valid, though 
non-printable, character, I would expect more `'\u0001'`.



Comment at: clang/test/SemaCXX/static-assert.cpp:274-277
+  static_assert((char)10 == '\0', ""); // expected-error {{failed}} \
+   // expected-note {{n' (0x0A, 10) == 
'' (0x00, 0)'}}
+  // The note above is intended to match "evaluates to '\n' (0x0A, 10) == 
'' (0x00, 0)'", but if we write it as it is,
+  // the "\n" cannot be consumed by the diagnostic consumer.

Here too, I find the `''` presentation surprising; either of `'\0'` or 
`'\u'` would be preferred.


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

https://reviews.llvm.org/D155610

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

[PATCH] D159345: [Clang] Handle non-ASCII after line splicing

[Tom Honermann via Phabricator via cfe-commits]

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

This looks good to me modulo a couple of nits.




Comment at: clang/include/clang/Lex/Lexer.h:806-807
   /// Try to consume an identifier character encoded in UTF-8.
   /// \param CurPtr Points to the start of the (potential) UTF-8 code unit
   ///sequence. On success, updated to point past the end of it.
   /// \return \c true if a UTF-8 sequence mapping to an acceptable identifier

`Tok` should be documented here.



Comment at: clang/lib/Lex/Lexer.cpp:1800
+  // calling ConsumeChar ensures the NeedsCleaning flag is set on the token
+  // being lexed, and that warnings about trailing spaces are emmitted.
+  ConsumeChar(CurPtr, FirstCodeUnitSize, Tok);




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159345

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

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Tom Honermann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6658db5e5014: [clang] Fix timing of propagation of 
MSInheritanceAttr for template… (authored by tahonermann).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158869

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp


Index: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
===
--- clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -fms-extensions | FileCheck -allow-deprecated-dag-overlap 
%s
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
+// RUN: %clang_cc1 -std=c++17 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -fms-extensions -verify
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -DMEMFUN -fms-extensions -verify
 
@@ -934,3 +935,32 @@
 void A::printd() { JSMethod(); }
 // CHECK-LABEL: 
@"??$JSMethod@VA@PMFInTemplateArgument@@$1?printd@12@AAEHH@Z@PMFInTemplateArgument@@YAXXZ"(
 }
+
+namespace MissingMSInheritanceAttr {
+// This is a regression test for an assertion failure that occurred when
+// compiling for C++17. The issue concerned a failure to propagate a
+// MSInheritanceAttr attribute for the explicit template instantiation
+// definition prior to it being required to complete the specialization
+// definition in order to determine its alignment so as to resolve a
+// lookup for a deallocation function for the virtual destructor.
+template 
+class a;
+struct b {
+  typedef void (a::*f)();
+  f d;
+};
+template 
+class a {
+  virtual ~a();
+  b e;
+};
+extern template class a;
+template class a;
+#ifdef _WIN64
+static_assert(sizeof(b::d) == 24, "");
+static_assert(sizeof(void (a::*)()) == 24, "");
+#else
+static_assert(sizeof(b::d) == 16, "");
+static_assert(sizeof(void (a::*)()) == 16, "");
+#endif
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -10110,6 +10110,17 @@
 ClassTemplate, CanonicalConverted, PrevDecl);
 SetNestedNameSpecifier(*this, Specialization, SS);
 
+// A MSInheritanceAttr attached to the previous declaration must be
+// propagated to the new node prior to instantiation.
+if (PrevDecl) {
+  if (const auto *A = PrevDecl->getAttr()) {
+auto *Clone = A->clone(getASTContext());
+Clone->setInherited(true);
+Specialization->addAttr(Clone);
+Consumer.AssignInheritanceModel(Specialization);
+  }
+}
+
 if (!HasNoEffect && !PrevDecl) {
   // Insert the new specialization.
   ClassTemplate->AddSpecialization(Specialization, InsertPos);
@@ -10226,11 +10237,6 @@
   dllExportImportClassTemplateSpecialization(*this, Def);
 }
 
-if (Def->hasAttr()) {
-  Specialization->addAttr(Def->getAttr());
-  Consumer.AssignInheritanceModel(Specialization);
-}
-
 // Set the template specialization kind. Make sure it is set before
 // instantiating the members which will trigger ASTConsumer callbacks.
 Specialization->setTemplateSpecializationKind(TSK);
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -292,6 +292,9 @@
 
 Windows Support
 ^^^
+- Fixed an assertion failure that occurred due to a failure to propagate
+  ``MSInheritanceAttr`` attributes to class template instantiations created
+  for explicit template instantiation declarations.
 
 LoongArch Support
 ^


Index: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
===
--- clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=i386-pc-win32 -fms-extensions | FileCheck -allow-deprecated-dag-overlap %s
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
+// RUN: %clang_cc1 -std=c++17 -Wno-uninitialized

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann updated this revision to Diff 555076.
tahonermann added a comment.

Added a release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158869

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp


Index: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
===
--- clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -fms-extensions | FileCheck -allow-deprecated-dag-overlap 
%s
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
+// RUN: %clang_cc1 -std=c++17 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -fms-extensions -verify
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -DMEMFUN -fms-extensions -verify
 
@@ -934,3 +935,32 @@
 void A::printd() { JSMethod(); }
 // CHECK-LABEL: 
@"??$JSMethod@VA@PMFInTemplateArgument@@$1?printd@12@AAEHH@Z@PMFInTemplateArgument@@YAXXZ"(
 }
+
+namespace MissingMSInheritanceAttr {
+// This is a regression test for an assertion failure that occurred when
+// compiling for C++17. The issue concerned a failure to propagate a
+// MSInheritanceAttr attribute for the explicit template instantiation
+// definition prior to it being required to complete the specialization
+// definition in order to determine its alignment so as to resolve a
+// lookup for a deallocation function for the virtual destructor.
+template 
+class a;
+struct b {
+  typedef void (a::*f)();
+  f d;
+};
+template 
+class a {
+  virtual ~a();
+  b e;
+};
+extern template class a;
+template class a;
+#ifdef _WIN64
+static_assert(sizeof(b::d) == 24, "");
+static_assert(sizeof(void (a::*)()) == 24, "");
+#else
+static_assert(sizeof(b::d) == 16, "");
+static_assert(sizeof(void (a::*)()) == 16, "");
+#endif
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -10110,6 +10110,17 @@
 ClassTemplate, CanonicalConverted, PrevDecl);
 SetNestedNameSpecifier(*this, Specialization, SS);
 
+// A MSInheritanceAttr attached to the previous declaration must be
+// propagated to the new node prior to instantiation.
+if (PrevDecl) {
+  if (const auto *A = PrevDecl->getAttr()) {
+auto *Clone = A->clone(getASTContext());
+Clone->setInherited(true);
+Specialization->addAttr(Clone);
+Consumer.AssignInheritanceModel(Specialization);
+  }
+}
+
 if (!HasNoEffect && !PrevDecl) {
   // Insert the new specialization.
   ClassTemplate->AddSpecialization(Specialization, InsertPos);
@@ -10226,11 +10237,6 @@
   dllExportImportClassTemplateSpecialization(*this, Def);
 }
 
-if (Def->hasAttr()) {
-  Specialization->addAttr(Def->getAttr());
-  Consumer.AssignInheritanceModel(Specialization);
-}
-
 // Set the template specialization kind. Make sure it is set before
 // instantiating the members which will trigger ASTConsumer callbacks.
 Specialization->setTemplateSpecializationKind(TSK);
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -292,6 +292,9 @@
 
 Windows Support
 ^^^
+- Fixed an assertion failure that occurred due to a failure to propagate
+  ``MSInheritanceAttr`` attributes to class template instantiations created
+  for explicit template instantiation declarations.
 
 LoongArch Support
 ^


Index: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
===
--- clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=i386-pc-win32 -fms-extensions | FileCheck -allow-deprecated-dag-overlap %s
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
+// RUN: %clang_cc1 -std=c++17 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno

[PATCH] D158293: [NFC][Clang] Fix static code analyzer concern about null value dereference

[Tom Honermann via Phabricator via cfe-commits]

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

Thanks, Soumi, looks good to me!


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

https://reviews.llvm.org/D158293

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

[PATCH] D158293: [NFC][Clang] Fix static code analyzer concern about null value dereference

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/Lex/PPDirectives.cpp:494-495
   ++NumSkipped;
   assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?");
+  assert(CurLexer && "Invalid lexer value");
 

Would you mind splitting out all of the predicates? The updates to the messages 
in the suggested edit are intended to match the style present in other similar 
assertions.


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

https://reviews.llvm.org/D158293

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

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann updated this revision to Diff 554413.
tahonermann marked an inline comment as done.
tahonermann edited the summary of this revision.
tahonermann added a comment.

Address prior review feedback.

This update also modifies the propagation of the `MSInheritanceAttr` attribute 
for explicit template instantiations to clone the attribute and to set it as 
inherited. Previously, the same `MSInheritanceAttr` object was being attached 
to each declaration. @erichkeane, it would be great if you could confirm that 
this is the correct behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158869

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp


Index: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
===
--- clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -fms-extensions | FileCheck -allow-deprecated-dag-overlap 
%s
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
+// RUN: %clang_cc1 -std=c++17 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -fms-extensions -verify
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -DMEMFUN -fms-extensions -verify
 
@@ -934,3 +935,32 @@
 void A::printd() { JSMethod(); }
 // CHECK-LABEL: 
@"??$JSMethod@VA@PMFInTemplateArgument@@$1?printd@12@AAEHH@Z@PMFInTemplateArgument@@YAXXZ"(
 }
+
+namespace MissingMSInheritanceAttr {
+// This is a regression test for an assertion failure that occurred when
+// compiling for C++17. The issue concerned a failure to propagate a
+// MSInheritanceAttr attribute for the explicit template instantiation
+// definition prior to it being required to complete the specialization
+// definition in order to determine its alignment so as to resolve a
+// lookup for a deallocation function for the virtual destructor.
+template 
+class a;
+struct b {
+  typedef void (a::*f)();
+  f d;
+};
+template 
+class a {
+  virtual ~a();
+  b e;
+};
+extern template class a;
+template class a;
+#ifdef _WIN64
+static_assert(sizeof(b::d) == 24, "");
+static_assert(sizeof(void (a::*)()) == 24, "");
+#else
+static_assert(sizeof(b::d) == 16, "");
+static_assert(sizeof(void (a::*)()) == 16, "");
+#endif
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -10105,6 +10105,17 @@
 ClassTemplate, CanonicalConverted, PrevDecl);
 SetNestedNameSpecifier(*this, Specialization, SS);
 
+// A MSInheritanceAttr attached to the previous declaration must be
+// propagated to the new node prior to instantiation.
+if (PrevDecl) {
+  if (const auto *A = PrevDecl->getAttr()) {
+auto *Clone = A->clone(getASTContext());
+Clone->setInherited(true);
+Specialization->addAttr(Clone);
+Consumer.AssignInheritanceModel(Specialization);
+  }
+}
+
 if (!HasNoEffect && !PrevDecl) {
   // Insert the new specialization.
   ClassTemplate->AddSpecialization(Specialization, InsertPos);
@@ -10221,11 +10232,6 @@
   dllExportImportClassTemplateSpecialization(*this, Def);
 }
 
-if (Def->hasAttr()) {
-  Specialization->addAttr(Def->getAttr());
-  Consumer.AssignInheritanceModel(Specialization);
-}
-
 // Set the template specialization kind. Make sure it is set before
 // instantiating the members which will trigger ASTConsumer callbacks.
 Specialization->setTemplateSpecializationKind(TSK);


Index: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
===
--- clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=i386-pc-win32 -fms-extensions | FileCheck -allow-deprecated-dag-overlap %s
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
+// RUN: %clang_cc1 -std=c++17 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - -triple=i386-pc-win32

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Tom Honermann via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaTemplate.cpp:10110-10111
+// propagated to the new node prior to instantiation.
+if (PrevDecl && PrevDecl->hasAttr()) {
+  Specialization->addAttr(PrevDecl->getAttr());
+  Consumer.AssignInheritanceModel(Specialization);

rnk wrote:
> `hasAttr()` is O(n) in the attribute list size, so I would recommend this 
> pattern:
> ```
>   if (PrevDecl) {
> if (const auto *A = PrevDecl->getAttr()) {
>   Specialization->addAttr(A);
>   ...
> ```
Sounds good, I'll make that change.



Comment at: clang/lib/Sema/SemaTemplate.cpp:10112
+  Specialization->addAttr(PrevDecl->getAttr());
+  Consumer.AssignInheritanceModel(Specialization);
+}

rnk wrote:
> tahonermann wrote:
> > erichkeane wrote:
> > > tahonermann wrote:
> > > > I am concerned that moving the call to 
> > > > `Consumer.AssignInheritanceModel()` to immediately after the creation 
> > > > of the node, but before it is populated might be problematic. 
> > > > Previously, this call was still made before the node was completely 
> > > > constructed (e.g., before `setTemplateSpecializationKind()` is called 
> > > > for it). It is difficult to tell if any consumers might be dependent on 
> > > > more of the definition being present. 
> > > SO I think we still need to do this off of the definition, right?  Else 
> > > if `PrevDecl` ends up being a declaration (followed later by a definition 
> > > that has the attribute), we'll end up missing it?  Typically attributes 
> > > are 'added' by later decls, so only the 'latest one' (though attributes 
> > > can't be added after definition IIRC) has 'them all'.
> > This handles the situation where a new node is created for either an 
> > explicit template instantiation declaration or definition; that matches 
> > prior behavior. The goal is to ensure each node, regardless of the reason 
> > for its creation, inherits the attribute from the previous declaration; 
> > that ensures that any explicitly declared attributes are checked for 
> > consistency (see `Sema::mergeMSInheritanceAttr()`).
> > 
> > Note that an explicit class template specialization is not allowed to 
> > follow an explicit template instantiation declaration or definition. 
> > https://godbolt.org/z/cbvaac717.
> I think it is safe to call `AssignInheritanceModel` earlier. We mostly just 
> use it to invalidate some clang AST -> LLVM IR type translation cache.
Great, thank you for confirming that.



Comment at: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp:958
+extern template class a;
+template class a;
+}

rnk wrote:
> My expectation here is that we assign the unknown inheritance model to 
> `a`, is that right? Can you add a static_assert to that effect, or add 
> some CHECK lines for the structure,  maybe make an alloca of type `void 
> (a::*var)()`  and check for the allocated type (it should be a struct 
> with a pointer with lots of i32s)?
Yes, when a pointer to member is formed for an incomplete class, in the absence 
of a `#pragma pointers_to_members` directive, use of inheritance keywords like 
`__single_inheritance`, or use of the `/vmb` or `/vmg` options (or 
equivalents), the "full_generality" / "virtual_inheritance" model is selected. 
I did verify that manually.

I can add a `static_assert` so long as it follows the explicit template 
instantiation definition. If it is placed earlier, then code related to 
obtaining a complete type is run and that ends up avoiding the assertion 
failure. See https://godbolt.org/z/qzTTfdfY1. This might indicate there is a 
bug elsewhere; I find it surprising that the early `static_assert` has that 
effect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158869

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

[PATCH] D158293: [NFC][Clang] Fix static code analyzer concern about null value dereference

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Lex/PPDirectives.cpp:494-495
   ++NumSkipped;
-  assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?");
+  assert(!CurTokenLexer && CurPPLexer && CurLexer &&
+ "Lexing a macro, not a file?");
 

I would prefer that this `assert` be split up so that, when/if a failure 
occurs, we'll be able to tell which predicate failed.


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

https://reviews.llvm.org/D158293

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

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/Sema/SemaTemplate.cpp:10112
+  Specialization->addAttr(PrevDecl->getAttr());
+  Consumer.AssignInheritanceModel(Specialization);
+}

erichkeane wrote:
> tahonermann wrote:
> > I am concerned that moving the call to `Consumer.AssignInheritanceModel()` 
> > to immediately after the creation of the node, but before it is populated 
> > might be problematic. Previously, this call was still made before the node 
> > was completely constructed (e.g., before `setTemplateSpecializationKind()` 
> > is called for it). It is difficult to tell if any consumers might be 
> > dependent on more of the definition being present. 
> SO I think we still need to do this off of the definition, right?  Else if 
> `PrevDecl` ends up being a declaration (followed later by a definition that 
> has the attribute), we'll end up missing it?  Typically attributes are 
> 'added' by later decls, so only the 'latest one' (though attributes can't be 
> added after definition IIRC) has 'them all'.
This handles the situation where a new node is created for either an explicit 
template instantiation declaration or definition; that matches prior behavior. 
The goal is to ensure each node, regardless of the reason for its creation, 
inherits the attribute from the previous declaration; that ensures that any 
explicitly declared attributes are checked for consistency (see 
`Sema::mergeMSInheritanceAttr()`).

Note that an explicit class template specialization is not allowed to follow an 
explicit template instantiation declaration or definition. 
https://godbolt.org/z/cbvaac717.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158869

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

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added reviewers: rnk, zequanwu.
tahonermann added a comment.

Adding Erich as attributes owner, and Reid and Zequan for MS ABI. This patch 
slightly modifies the changes made via D94646  
and committed as rG4fffbc150cca1638051b8ad2a20f4b8240df0869 
 for 
GH48031 .




Comment at: clang/lib/Sema/SemaTemplate.cpp:10112
+  Specialization->addAttr(PrevDecl->getAttr());
+  Consumer.AssignInheritanceModel(Specialization);
+}

I am concerned that moving the call to `Consumer.AssignInheritanceModel()` to 
immediately after the creation of the node, but before it is populated might be 
problematic. Previously, this call was still made before the node was 
completely constructed (e.g., before `setTemplateSpecializationKind()` is 
called for it). It is difficult to tell if any consumers might be dependent on 
more of the definition being present. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158869

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

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann created this revision.
tahonermann added reviewers: erichkeane, aaron.ballman.
Herald added a project: All.
tahonermann requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This change addresses two issues:

1. A failure to propagate a `MSInheritanceAttr` prior to it being required by 
an explicit class template instantiation definition.
2. The same `MSInheritanceAttr` attribute being attached to the same 
`ClassTemplateSpecializationDecl` twice.

`Sema::ActOnExplicitInstantiation()` is responsible for the construction of 
`ClassTemplateSpecializationDecl` nodes for explicit template instantiation 
declarations and definitions.  When invoked when a prior declaration node 
corresponding to an implicit instantiation exists, the prior declaration node 
is repurposed to represent the explicit instantiation declaration or 
definition.  When no previous declaration node exists or when the previous node 
corresponds to an explicit declaration, a new node is allocated. Previously, in 
either case, the function attempted to propagate any existing 
`MSInheritanceAttr` attribute from the previous node, but did so regardless of 
whether the previous node was reused (in which case the repurposed previous 
node would gain a second attachment of the attribute; the second issue listed 
above) or a new node was created.  In the latter case, the attribute was not 
propagated before it was required to be present when compiling for C++17 or 
later (the first issue listed above).  The absent attribute resulted in an 
assertion failure that occurred during instantiation of the specialization 
definition when attempting to complete the definition in order to determine its 
alignment so as to resolve a lookup for a deallocation function for a virtual 
destructor.  This change addresses both issues by propagating the attribute 
closer in time to when a new `ClassTemplateSpecializationDecl` node is created 
and only when such a node is newly created.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158869

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp


Index: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
===
--- clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -fms-extensions | FileCheck -allow-deprecated-dag-overlap 
%s
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
+// RUN: %clang_cc1 -std=c++17 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=x86_64-pc-win32 -fms-extensions | FileCheck %s -check-prefix=X64
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -fms-extensions -verify
 // RUN: %clang_cc1 -std=c++11 -Wno-uninitialized -fno-rtti -emit-llvm %s -o - 
-triple=i386-pc-win32 -DINCOMPLETE_VIRTUAL -DMEMFUN -fms-extensions -verify
 
@@ -934,3 +935,25 @@
 void A::printd() { JSMethod(); }
 // CHECK-LABEL: 
@"??$JSMethod@VA@PMFInTemplateArgument@@$1?printd@12@AAEHH@Z@PMFInTemplateArgument@@YAXXZ"(
 }
+
+namespace MissingMSInheritanceAttr {
+// This is a regression test for an assertion failure that occurred when
+// compiling for C++17. The issue concerned a failure to propagate a
+// MSInheritanceAttr attribute for the explicit template instantiation
+// definition prior to it being required to complete the specialization
+// definition in order to determine its alignment so as to resolve a
+// lookup for a deallocation function for the virtual destructor.
+template 
+class a;
+class b {
+  typedef void (a::*f)();
+  f d;
+};
+template 
+class a {
+  virtual ~a();
+  b e;
+};
+extern template class a;
+template class a;
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -10105,6 +10105,13 @@
 ClassTemplate, CanonicalConverted, PrevDecl);
 SetNestedNameSpecifier(*this, Specialization, SS);
 
+// A MSInheritanceAttr attached to the previous declaration must be
+// propagated to the new node prior to instantiation.
+if (PrevDecl && PrevDecl->hasAttr()) {
+  Specialization->addAttr(PrevDecl->getAttr());
+  Consumer.AssignInheritanceModel(Specialization);
+}
+
 if (!HasNoEffect && !PrevDecl) {
   // Insert the new specialization.
   ClassTemplate->AddSpecialization(Specialization, InsertPos);
@@ -10221,11 +10228,6 @@
   dllExportImportClassTemplateSpecialization(*this, Def);
 }
 
-if (Def->hasAttr()) {
-  Specialization->addAttr(Def->getAttr());
-  Consumer.Assign

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

I think it would also be a good idea to exercise the test case for a target 
that lacks ifunc support to ensure that inter-TU calls work as expected.


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

https://reviews.llvm.org/D158666

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

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

A couple of observations with regard to compatibility:

gcc,, at least by default, emits the TC implementations as local functions, the 
resolver as a weak global function, and the undecorated name as an ifunc. When 
only a TC declaration (not a definition) is present, gcc still emits the 
resolver and ifunc, but a failure then occurs at line time since the 
implementation symbols referenced by the resolver function cannot be resolved 
to the local symbols emitted for a TU with the TC definitions. I can only guess 
that this is intentional; that inter-TU calls are intended to be dispatched via 
the ifunc+resolver and that TC use is therefore restricted to TUs that have 
definitions. Clang, at least by default, emits the TC implementations as global 
functions that can be resolved for direct reference by other TUs. This could be 
seen as potentially exposing implementation details (e.g., the author of the TC 
functions might reserve the right to change the list of targets without 
worrying about breaking usage by other TUs.

For TUs that only have a declaration of the function without TC, it will be 
necessary to recompile the TUs that define TC functions no matter what. This is 
because the calling TU (that has neither a declaration nor a definition of the 
function with TC applied) cannot know to dispatch to the ifunc+resolver and 
cannot provide a definition for the undecorated function name (not even an 
alias since it doesn't know to do so).


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

https://reviews.llvm.org/D158666

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

[PATCH] D158522: [NFC][CLANG] Fix static analyzer bugs about large copy by values

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

Looks fine to me!


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

https://reviews.llvm.org/D158522

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

[PATCH] D158293: [NFC][Clang] Fix static code analyzer concern about null value dereference

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Lex/PPDirectives.cpp:494
   ++NumSkipped;
   assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?");
 

Perhaps it would make sense to assert `CurLexer` here as well.



Comment at: clang/lib/Lex/PPDirectives.cpp:555
   while (true) {
 CurLexer->Lex(Tok);
 

I don't think this change is sufficient. If `CurLexer` is null, then the `else` 
branch will be entered and an unconditional dereference occurs there as well. 
It looks like more analysis is needed, but perhaps the correct fix is to add a 
non-null assertion somewhere above.


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

https://reviews.llvm.org/D158293

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

[PATCH] D158372: [Clang] Treat invalid UDL as two tokens

[Tom Honermann via Phabricator via cfe-commits]

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

Given Richard's comments, it seems that changes are needed.


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

https://reviews.llvm.org/D158372

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

[PATCH] D158488: [NFC] Initialize member pointers to nullptr.

[Tom Honermann via Phabricator via cfe-commits]

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

Looks good to me.




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:934
   // Return value of the runtime offloading call.
-  Value *Return;
+  Value *Return = nullptr;
 

aaron.ballman wrote:
> This is necessary to initialize because `emitTargetKernel()` has an early 
> return which does not initialize the passed reference to this object.
`Return` is passed to `Builder.CreateIsNotNull()`. I briefly inspected and it 
looks like it handles a null argument, so I agree this looks like the right fix.


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

https://reviews.llvm.org/D158488

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

[PATCH] D158285: [NFC][CLANG] Fix wrong orders of function arguments positions

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/AST/Interp/InterpBlock.cpp:94-95
 
 DeadBlock::DeadBlock(DeadBlock *&Root, Block *Blk)
-: Root(Root), B(Blk->Desc, Blk->IsStatic, Blk->IsExtern, /*isDead=*/true) {
+: Root(Root), B(Blk->Desc, Blk->IsExtern, Blk->IsStatic, /*isDead=*/true) {
   // Add the block to the chain of dead blocks.

steakhal wrote:
> I think this is a TP.
> I find this unfortunate, that while all the ˙Block` ctors accepts the 
> parameters in the order how the backing fields are defined - except for the 
> `isDead` ctor overload, where the `IsExtern` and `IsStatic`.
> 
> To address this, I'd recommend not swapping the parameters at the callsite, 
> but rather fix the ctor overload to expect the parameters in the right order 
> as the rest of the ctors do. And of course, check all the callsites to this 
> weird constructor to fix them as well.
> 
> WDYT @tbaeder
I would be surprised if it makes sense for both `IsExtern` and `IsStatic` to be 
true. Perhaps `Block` could be modified to hold a single data member of type 
`StorageClass` that is asserted to be one of `SC_None`, `SC_Extern`, or 
`SC_Static`?



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1468-1477
   std::optional getRangeForNegatedSymSym(const SymSymExpr *SSE) {
 return getRangeForNegatedExpr(
 [SSE, State = this->State]() -> SymbolRef {
   if (SSE->getOpcode() == BO_Sub)
 return State->getSymbolManager().getSymSymExpr(
-SSE->getRHS(), BO_Sub, SSE->getLHS(), SSE->getType());
+SSE->getLHS(), BO_Sub, SSE->getRHS(), SSE->getType());
   return nullptr;

steakhal wrote:
> Now this is within my realm.
> This code applies the following transformation: `-(X - Y)  =>  (Y - X)` .
> Consequently, this is intentional.
Ideally, this change would have tripped up a test. Do we have tests that 
attempt to verify source locations such that one could be added? I know testing 
source locations is difficult and tedious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158285

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

[PATCH] D158488: [NFC] Initialize member pointers to nullptr.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:2606-2607
   enum { INIT, CONDVAR, COND, INC, BODY, END_EXPR };
-  Stmt* SubExprs[END_EXPR]; // SubExprs[INIT] is an expression or declstmt.
+  Stmt *SubExprs[END_EXPR] = {
+  nullptr}; // SubExprs[INIT] is an expression or declstmt.
   SourceLocation LParenLoc, RParenLoc;

aaron.ballman wrote:
> I don't think this initialization is necessary. The constructor for `ForStmt` 
> initializes all of the valid elements: 
> https://github.com/llvm/llvm-project/blob/87c5a3e203ae3643bc13c9a13917b92a657f4358/clang/lib/AST/Stmt.cpp#L1024
> 
> The `EmptyShell` constructor does not initialize anything but that's because 
> it is piecemeal initialized by the AST importer, but all of its fields are 
> also initialized: 
> https://github.com/llvm/llvm-project/blob/87c5a3e203ae3643bc13c9a13917b92a657f4358/clang/lib/Serialization/ASTReaderStmt.cpp#L296
Declarations like this are common in the AST. I'm curious while static analysis 
flagged this one in particular. Perhaps it identified a path where one or more 
of the elements don't get initialized? If so, this would be a good find and a 
fix should be applied to that path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158488

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

[PATCH] D155776: [NFC] Add checks for self-assignment.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

Thank you for following up, Sindhu! Looks good!


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

https://reviews.llvm.org/D155776

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

[PATCH] D158285: [NFC][CLANG] Fix wrong orders of function arguments positions

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> To me, an important metric of a quality patch is if the title and the summary 
> describe why we have this patch, and what we do about it.

@steakhal, I'll work with the people submitting these patches to add more 
context to the title, summary, and commit comments. I agree that would be 
helpful.

Ideally, we would copy the report from the static analysis tool, but we have 
been informed that we are not allowed to divulge the tool we are using, so we 
aren't permitted to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158285

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

[PATCH] D158293: [NFC][CLANG] Fix potential dereferencing of null return values

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2009-2010
(Line.MightBeFunctionDecl || Line.InPPDirective) &&
-   Current.NestingLevel == 0 &&
+   Current.NestingLevel == 0 && Current.Previous &&
!Current.Previous->isOneOf(tok::kw_operator, tok::identifier)) {
   // not auto operator->() -> xxx;

owenpan wrote:
> `Current.Previous` can't be null here because `AutoFound` is `true`.
Could you please elaborate on why you believe it is safe to move the check of 
`Current.Previous` inside the body of the `if` statement? Doing so will short 
circuit the remaining `else if` cases such that `Current.setType()` will not be 
called at all. It isn't obvious to me that those cases should not be considered 
if the previous token was not one of `kw_operator` or `identifier`. This looks 
like it has potential to change behavior.

The change that was originally proposed is clearly safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158293

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

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9
+// Note: We must use the real path here, because the logic to detect case
+// mismatch relies on resolving the real path and checking that casing differs.
+// If we use %t and we are on a substitute drive S: mapping to C:\subst,
+// then we will compare "S:\test.dir\FOO.h" to "C:\subst\test.dir\foo.h"
+// and avoid emitting the diagnostic because the structure is different.

MrTrillian wrote:
> tahonermann wrote:
> > MrTrillian wrote:
> > > tahonermann wrote:
> > > > MrTrillian wrote:
> > > > > tahonermann wrote:
> > > > > > Hmm, is it really desirable or necessary that the case mismatch 
> > > > > > logic resolve substituted drives? I wouldn't think so. This seems 
> > > > > > like another case where our common canonicalization would be 
> > > > > > desired.
> > > > > I think it is necessary as I don't see how else we can retrieve the 
> > > > > original casing of the file path. Merely making the path absolute 
> > > > > would not do that. Searching for solutions on the internet finds 
> > > > > ideas that are worse like converting to a short path then back to a 
> > > > > long path or rebuilding the path one component at a time with a 
> > > > > series directory listing requests.
> > > > The warning this test checks for is diagnosed in 
> > > > `Preprocessor::HandleHeaderIncludeOrImport()`; search for 
> > > > `pp_nonportable_path` and/or `pp_nonportable_system_path`. I believe 
> > > > this warning will be issued if any component of the path does not match 
> > > > the case of the include directive. Since the file name differs in case, 
> > > > unless there is a bug in handling of the file name, this test isn't 
> > > > sensitive to case mismatches in `%t`.
> > > > 
> > > > From a user point of view, resolving a substitute drive doesn't seem 
> > > > desirableto me with respect to determining whether a non-portable path 
> > > > is being used; I don't think the behavior should be dependent on 
> > > > whether a substitute drive is being used or what its target is.
> > > @tahonermann I think the code is working by design and it would be an 
> > > unrelated change to modify its logic. See `trySimplifyPath` in 
> > > `PPDirectives.cpp`:
> > > 
> > > ```
> > >   // Below is a best-effort to handle ".." in paths. It is admittedly
> > >   // not 100% correct in the presence of symlinks.
> > > 
> > > // If these path components differ by more than just case, then we
> > > // may be looking at symlinked paths. Bail on this diagnostic to 
> > > avoid
> > > // noisy false positives.
> > > ```
> > > 
> > > The test was previously implicitly requiring getting the realpath, and it 
> > > worked because `lit.py`'s logic of doing that. Now that requirement is 
> > > explicit in the test.
> > I'm still not following here. Are you saying that `trySimplifyPath()` will 
> > replace substitute drives? If so, that doesn't sound desirable and I would 
> > expect it to be problematic for your use case. I think we should follow 
> > this up ... somewhere (now that this review is closed).
> @tahonermann . `trySimplifyPath()` does not replace substitute drives. It's a 
> best-effort attempt to see if the included path mismatches the real file path 
> only by case. It explicitly bails out without diagnostics if it finds that 
> the included path has a different shape from the real file path, which will 
> happen if the included path is on a substitute drive. It has to compare with 
> the real file path because this is the only reasonable way to get the 
> original path casing.
> 
> It was already the case that this diagnostic bailed out in the presence of 
> symbolic links, so there are no behavioral differences. I needed to update 
> the test because previously `lit.py` would enforce that `%t` was a real path, 
> and now it doesn't, which means that we would hit the "bail out" code path 
> and not produce the diagnostic.
I think `trySimplifyPath()` is not particularly relevant as it just performs a 
simple canonicalization (removal of `.` and `..` path components without regard 
for symlink traversal) and case insensitive comparison with the remaining 
components with a "real" path that is presumed to already be devoid of such 
components. It is therefore sensitive to structure, but only for the (non-`.` 
and non-`..`) components present in the (non-real) `Components` vector; the 
"real" path may have more leading components (the `Components` vector may 
represent a relative path). The presence of a substitute drive in the "real" 
path won't contribute to a structural difference unless the `Components` vector 
is absolute but with a drive other than the substitute drive or if it is 
relative but starting at a higher directory level than the substitute drive; 
neither of which should be the case for this test when `%t` is consistently 
used.

The only relevant user path pro

[PATCH] D157989: [NFC] Initialize pointer field

[Tom Honermann via Phabricator via cfe-commits]

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

Looks good to me. I see that `OpenMPIRBuilder::emitTargetKernel()` can return 
without having assigned the `Return` parameter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157989

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

[PATCH] D157888: [NFC][Clang] Fix static analyzer concern about null value dereference

[Tom Honermann via Phabricator via cfe-commits]

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

Looks good to me!


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

https://reviews.llvm.org/D157888

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

[PATCH] D157554: [NFC][Clang] Fix static analyzer concern about null pointer dereference

[Tom Honermann via Phabricator via cfe-commits]

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

Looks good to me!


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

https://reviews.llvm.org/D157554

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

[PATCH] D157885: [NFC][Clang] Fix static analyzer concern about null value derefence

[Tom Honermann via Phabricator via cfe-commits]

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

Looks fine to me!


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

https://reviews.llvm.org/D157885

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

[PATCH] D155081: Specify the developer policy around links to external resources

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

This looks good to me.


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

https://reviews.llvm.org/D155081

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

[PATCH] D157454: [NFC][Clang] Fix static analyzer concern about null value dereference

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

Looks good to me. Thanks, Elizabeth!


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

https://reviews.llvm.org/D157454

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

[PATCH] D157454: [NFC][Clang] Fix static analyzer concern about null value dereference

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/CodeGen/CGObjC.cpp:222-223
   Selector Sel = MethodWithObjects->getSelector();
-  QualType ResultType = E->getType();
-  const ObjCObjectPointerType *InterfacePointerType
-= ResultType->getAsObjCInterfacePointerType();
+  const ObjCObjectPointerType *InterfacePointerType =
+  cast(E->getType());
   ObjCInterfaceDecl *Class

eandrews wrote:
> tahonermann wrote:
> > The previous code included a guarantee that 
> > `InterfacePointerType->getInterfaceType()` is non-null and this change 
> > loses that assurance. Presumably, we never ran into a violation of that 
> > guarantee in the past (since a SIGSEGV would likely have occurred below 
> > otherwise), but perhaps we should consider an assertion to ensure that 
> > guarantee is still met.
> Hmmm... I guess in that case it was just make more sense to keep the old code 
> and add an assert for InterfacePointerType?
Ah, yes, I think that is a better approach!


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

https://reviews.llvm.org/D157454

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

[PATCH] D157454: [NFC][Clang] Fix static analyzer concern about null value dereference

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/CodeGen/CGObjC.cpp:222-223
   Selector Sel = MethodWithObjects->getSelector();
-  QualType ResultType = E->getType();
-  const ObjCObjectPointerType *InterfacePointerType
-= ResultType->getAsObjCInterfacePointerType();
+  const ObjCObjectPointerType *InterfacePointerType =
+  cast(E->getType());
   ObjCInterfaceDecl *Class

The previous code included a guarantee that 
`InterfacePointerType->getInterfaceType()` is non-null and this change loses 
that assurance. Presumably, we never ran into a violation of that guarantee in 
the past (since a SIGSEGV would likely have occurred below otherwise), but 
perhaps we should consider an assertion to ensure that guarantee is still met.


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

https://reviews.llvm.org/D157454

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

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added a comment.

I agree with the analysis done by @steakhal. The static analyzer being used 
(which I'm not allowed to name) is being too noisy in these cases; it complains 
about every case where an object of class type is copied regardless of size. An 
issue has been reported to the static analysis vendor about this. These 
complaints from the static analyzer will need to be moderated until the vendor 
satisfactorily addresses the reported issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157129

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

[PATCH] D157118: [NFC][Clang] Fix static analyzer concerns

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

I think this change is good as is, but I added some additional thoughts to 
Aaron's earlier comment.




Comment at: clang/lib/AST/StmtPrinter.cpp:178
 void StmtPrinter::PrintRawCompoundStmt(CompoundStmt *Node) {
+  assert(Node && "Compound statement cannot be null");
   OS << "{" << NL;

aaron.ballman wrote:
> Hmmm, I think this is what we're effectively already hoping is the case 
> today, but I don't think that's a safe assertion by itself. Consider: 
> https://github.com/llvm/llvm-project/blob/cd29ebb862b5c7a81c9f39c8c493f9246d6e5f0b/clang/lib/AST/StmtPrinter.cpp#L602
> 
> It might be worth looking at all the calls to `PrintRawCompoundStmt()` to see 
> which ones potentially can pass null in, and decide if there are additional 
> changes to make. e.g., should that `dyn_cast` be a `cast` instead so it 
> cannot return nullptr and we get the assertion a little bit earlier when 
> calling `cast<>`?
Another possibility would be to modify `ObjCAtFinallyStmt` to declare 
`AtFinallyStmt` as `CompoundStmt*` and to modify `getFinallyBody()` and 
`setFinallyBody()` accordingly. That would remove the need for that `dyn_cast` 
in `StmtPrinter::VisitObjCAtTryStmt()`. This would then require additional 
changes such as updates to `ASTStmtReader::VisitObjCAtFinallyStmt()` to perform 
a cast as is done in `ASTStmtReader::VisitStmtExpr()`. But none of that 
actually addresses the fact that this function has a precondition that `Node` 
is not null.


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

https://reviews.llvm.org/D157118

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

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> @tahonermann are all your comments addressed at this point?

Apologies for the late response; I was on vacation last week.

No. I'm still skeptical that there is ever a desire to expand substitute drives.




Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9
+// Note: We must use the real path here, because the logic to detect case
+// mismatch relies on resolving the real path and checking that casing differs.
+// If we use %t and we are on a substitute drive S: mapping to C:\subst,
+// then we will compare "S:\test.dir\FOO.h" to "C:\subst\test.dir\foo.h"
+// and avoid emitting the diagnostic because the structure is different.

MrTrillian wrote:
> tahonermann wrote:
> > MrTrillian wrote:
> > > tahonermann wrote:
> > > > Hmm, is it really desirable or necessary that the case mismatch logic 
> > > > resolve substituted drives? I wouldn't think so. This seems like 
> > > > another case where our common canonicalization would be desired.
> > > I think it is necessary as I don't see how else we can retrieve the 
> > > original casing of the file path. Merely making the path absolute would 
> > > not do that. Searching for solutions on the internet finds ideas that are 
> > > worse like converting to a short path then back to a long path or 
> > > rebuilding the path one component at a time with a series directory 
> > > listing requests.
> > The warning this test checks for is diagnosed in 
> > `Preprocessor::HandleHeaderIncludeOrImport()`; search for 
> > `pp_nonportable_path` and/or `pp_nonportable_system_path`. I believe this 
> > warning will be issued if any component of the path does not match the case 
> > of the include directive. Since the file name differs in case, unless there 
> > is a bug in handling of the file name, this test isn't sensitive to case 
> > mismatches in `%t`.
> > 
> > From a user point of view, resolving a substitute drive doesn't seem 
> > desirableto me with respect to determining whether a non-portable path is 
> > being used; I don't think the behavior should be dependent on whether a 
> > substitute drive is being used or what its target is.
> @tahonermann I think the code is working by design and it would be an 
> unrelated change to modify its logic. See `trySimplifyPath` in 
> `PPDirectives.cpp`:
> 
> ```
>   // Below is a best-effort to handle ".." in paths. It is admittedly
>   // not 100% correct in the presence of symlinks.
> 
> // If these path components differ by more than just case, then we
> // may be looking at symlinked paths. Bail on this diagnostic to avoid
> // noisy false positives.
> ```
> 
> The test was previously implicitly requiring getting the realpath, and it 
> worked because `lit.py`'s logic of doing that. Now that requirement is 
> explicit in the test.
I'm still not following here. Are you saying that `trySimplifyPath()` will 
replace substitute drives? If so, that doesn't sound desirable and I would 
expect it to be problematic for your use case. I think we should follow this up 
... somewhere (now that this review is closed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

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

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

[Tom Honermann via Phabricator via cfe-commits]

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

> You are absolutely right, -fno-coroutines would totally work for us, had it 
> been available.

Good. Gcc handles `-fcoroutines` and `-fno-coroutines` as I would expect 
(https://godbolt.org/z/7zEMd7cdW), so matching gcc behavior makes sense in any 
case. I would expect implementation to be quite straight forward, so I 
recommend we head in that direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156247

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

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

At first, it sounded to me like the option that would best suit Google's needs 
is `-fno-coroutines`. However:

- I see that Google does have some limited use of coroutines; perhaps that 
could be enabled on a per-TU or project basis by the build system? However...
- The `-fno-coroutines` option doesn't seem to work; 
https://godbolt.org/z/Phhqjd9so (Nor does `-fcoroutines` work to enable 
standard library coroutine features in pre-C++20 language modes).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156247

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

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

I'm happy with this. I noted one last rename suggestion to maintain 
consistency, but am accepting. Please give Corentin a final chance to raise any 
concerns. Thank you for being so responsive through all the review iterations!




Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1210
 def MicrosoftInitFromPredefined : DiagGroup<"microsoft-init-from-predefined">;
+def MicrosoftStringLiteralFromPredefined : 
DiagGroup<"microsoft-concat-predefined">;
 

We should probably name the diag group to match.
  - microsoft-concat-predefined -> microsoft-string-literal-from-predefined


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1981
+  // parsed yet).
+  assert(getLangOpts().MicrosoftExt);
+

RIscRIpt wrote:
> I think I should remove this assertion so this function would be usable 
> without MS ext, on the other hand it would be a noop without MS ext.
I suggest leaving it in solely because use of the function does impose some 
overhead in the construction of the `std::vector` that is returned. If that 
overhead can be avoided (I guess by passing the container to populate by 
reference as a separate argument and then return either the original `ArrayRef` 
or a new one referencing the populated container), then this could be made a 
no-op and the checks for `MicrosoftExt` at the call sites could be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

I think I'm happy with this. I noted two code changes that I think are no 
longer needed and offered some final suggestions on some names.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:118-120
+def ext_expand_predef_ms : ExtWarn<
+  "expansion of predefined identifier '%0' to a string literal is a Microsoft 
extension">,
+  InGroup;

For consistency with other names, I'll suggest some alternative names here. I 
think these better reflect the actual diagnostic message.
  - ext_expand_predef_ms -> ext_string_literal_from_predefined
  - MicrosoftConcatPredefined -> MicrosoftStringLiteralFromPredefined



Comment at: clang/include/clang/Basic/TokenKinds.h:22-23
 
+class LangOptions;
+
 namespace tok {

It looks like this change is no longer needed.



Comment at: clang/lib/Basic/TokenKinds.cpp:14-15
 #include "clang/Basic/TokenKinds.h"
+
+#include "clang/Basic/LangOptions.h"
 #include "llvm/Support/ErrorHandling.h"

This change appears to no longer be needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:2010-2013
+OS << "R\"EFLPM("
+   << PredefinedExpr::ComputeName(getPredefinedExprKind(Tok.getKind()),
+  currentDecl)
+   << ")EFLPM\"";

cor3ntin wrote:
> tahonermann wrote:
> > RIscRIpt wrote:
> > > tahonermann wrote:
> > > > "EFLPM"? I'm assuming this is an attempt to avoid an accidental clash 
> > > > with the computed name. Does this suffice to ensure a clash can't 
> > > > happen? If not, can we do better? Per 
> > > > http://eel.is/c++draft/lex.string#nt:r-char and 
> > > > http://eel.is/c++draft/lex.charset#1, we have a lot of flexibility 
> > > > regarding which characters to use.
> > > At first I thought you were hinting me to use non-ascii characters for 
> > > d-char-sequence. However, when I looked closely d-char-sequence is not as 
> > > flexible as r-chars that you referenced: 
> > > http://eel.is/c++draft/lex.string#nt:d-char and 
> > > http://eel.is/c++draft/lex.charset#2
> > > 
> > > Here're my thoughts:
> > > 1. I was afraid that there could be a possibility of a function name 
> > > contain either quote (`"`) or a backslash (`\`) despite it seemed 
> > > impossible.
> > > 2. At first I used `Lexer::Stringify` to make it bullet-proof. I didn't 
> > > like it, neither did you (reviewers).
> > > 3. I replaced `Lexer::Stringify` with raw-string-literal (d-char-sequence 
> > > was chosen as acronym of current function name).
> > > 4. Current `EFLPM` looks weird and cryptic. And we are limited to 
> > > [lex.charset.basic] with exceptions (space and earlier chars are not 
> > > allowed). I think, using a raw-string-literal without d-char-sequence 
> > > would be enough, because problem could be caused only by two consecutive 
> > > characters `)"`, neither of which can appear in a function name.
> > Sorry about that; you are absolutely right that `d-char-sequence` is (much) 
> > more constrained than `r-char-sequence`.
> > 
> > For `__FUNCSIG__`, the generated text can include arbitrary text. For 
> > example, given this declaration:
> >   void f(decltype(sizeof("()"))*)
> > the macro value is:
> >   void __cdecl f(decltype(sizeof ("()")) *)
> > which does contain `)"`.
> > 
> > I think we should do more to avoid any possibility of the resulting string 
> > being unparseable because the resulting diagnostics will be completely 
> > inscrutable. The best way to do that would be to avoid trying to produce a 
> > parseable string literal in the first place. Based on that and given that 
> > `Sema::ActOnStringLiteral()` immediately uses `StringLiteralParser` to 
> > produce an expanded string literal, I think we would be better off moving 
> > this expansion there such that these macros can be expanded to already 
> > cooked string literals that don't need to be parsed at all. This will mean 
> > having to modify the `StringLiteralParser` constructor to accept a `Decl*` 
> > pointer that will be passed `getCurLocalScopeDecl()` in 
> > `Sema::ActOnStringLiteral()`. `StringLiteralParser` can then assert if it 
> > ever encounters one of these function local predefined tokens when it 
> > hasn't been provided a corresponding `Decl*` to use with them.
> I didn't think about that scenario but in that case the original idea to use 
> Lexer::Stringify seems like a cleaner solution to me.
> Adding that kind of logic to `LiteralSupport` creates a lot of coupling for 
> not much gain that I can see.
> 
> 
> In that light, `Lexer::Stringify` seems like the least worse option.
Yeah, that might be the simpler solution. That matches how `__FILE__`, 
`__BASE_FILE__`, and `__FILE_NAME__` are handled in 
`Preprocessor::ExpandBuiltinMacro()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914

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

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> Marking comments as resolved per my reply. I'm not sure if that's best 
> practice!

Yes, that is fine.




Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9
+// Note: We must use the real path here, because the logic to detect case
+// mismatch relies on resolving the real path and checking that casing differs.
+// If we use %t and we are on a substitute drive S: mapping to C:\subst,
+// then we will compare "S:\test.dir\FOO.h" to "C:\subst\test.dir\foo.h"
+// and avoid emitting the diagnostic because the structure is different.

MrTrillian wrote:
> tahonermann wrote:
> > Hmm, is it really desirable or necessary that the case mismatch logic 
> > resolve substituted drives? I wouldn't think so. This seems like another 
> > case where our common canonicalization would be desired.
> I think it is necessary as I don't see how else we can retrieve the original 
> casing of the file path. Merely making the path absolute would not do that. 
> Searching for solutions on the internet finds ideas that are worse like 
> converting to a short path then back to a long path or rebuilding the path 
> one component at a time with a series directory listing requests.
The warning this test checks for is diagnosed in 
`Preprocessor::HandleHeaderIncludeOrImport()`; search for `pp_nonportable_path` 
and/or `pp_nonportable_system_path`. I believe this warning will be issued if 
any component of the path does not match the case of the include directive. 
Since the file name differs in case, unless there is a bug in handling of the 
file name, this test isn't sensitive to case mismatches in `%t`.

From a user point of view, resolving a substitute drive doesn't seem 
desirableto me with respect to determining whether a non-portable path is being 
used; I don't think the behavior should be dependent on whether a substitute 
drive is being used or what its target is.


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

https://reviews.llvm.org/D154130

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/test/Sema/ms_predefined_expr.cpp:156
+static_assert(__FUNCTION__ ""_len == 14); // expected-warning{{string 
literal concatenation of predefined identifier '__FUNCTION__' is a Microsoft 
extension}}
 }

Here is another case to test. MSVC accepts this.
  void test_static_assert() {
static_assert(true, __FUNCTION__);
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:2010-2013
+OS << "R\"EFLPM("
+   << PredefinedExpr::ComputeName(getPredefinedExprKind(Tok.getKind()),
+  currentDecl)
+   << ")EFLPM\"";

RIscRIpt wrote:
> tahonermann wrote:
> > "EFLPM"? I'm assuming this is an attempt to avoid an accidental clash with 
> > the computed name. Does this suffice to ensure a clash can't happen? If 
> > not, can we do better? Per http://eel.is/c++draft/lex.string#nt:r-char and 
> > http://eel.is/c++draft/lex.charset#1, we have a lot of flexibility 
> > regarding which characters to use.
> At first I thought you were hinting me to use non-ascii characters for 
> d-char-sequence. However, when I looked closely d-char-sequence is not as 
> flexible as r-chars that you referenced: 
> http://eel.is/c++draft/lex.string#nt:d-char and 
> http://eel.is/c++draft/lex.charset#2
> 
> Here're my thoughts:
> 1. I was afraid that there could be a possibility of a function name contain 
> either quote (`"`) or a backslash (`\`) despite it seemed impossible.
> 2. At first I used `Lexer::Stringify` to make it bullet-proof. I didn't like 
> it, neither did you (reviewers).
> 3. I replaced `Lexer::Stringify` with raw-string-literal (d-char-sequence was 
> chosen as acronym of current function name).
> 4. Current `EFLPM` looks weird and cryptic. And we are limited to 
> [lex.charset.basic] with exceptions (space and earlier chars are not 
> allowed). I think, using a raw-string-literal without d-char-sequence would 
> be enough, because problem could be caused only by two consecutive characters 
> `)"`, neither of which can appear in a function name.
Sorry about that; you are absolutely right that `d-char-sequence` is (much) 
more constrained than `r-char-sequence`.

For `__FUNCSIG__`, the generated text can include arbitrary text. For example, 
given this declaration:
  void f(decltype(sizeof("()"))*)
the macro value is:
  void __cdecl f(decltype(sizeof ("()")) *)
which does contain `)"`.

I think we should do more to avoid any possibility of the resulting string 
being unparseable because the resulting diagnostics will be completely 
inscrutable. The best way to do that would be to avoid trying to produce a 
parseable string literal in the first place. Based on that and given that 
`Sema::ActOnStringLiteral()` immediately uses `StringLiteralParser` to produce 
an expanded string literal, I think we would be better off moving this 
expansion there such that these macros can be expanded to already cooked string 
literals that don't need to be parsed at all. This will mean having to modify 
the `StringLiteralParser` constructor to accept a `Decl*` pointer that will be 
passed `getCurLocalScopeDecl()` in `Sema::ActOnStringLiteral()`. 
`StringLiteralParser` can then assert if it ever encounters one of these 
function local predefined tokens when it hasn't been provided a corresponding 
`Decl*` to use with them.



Comment at: clang/test/Sema/ms_predefined_expr.cpp:115
+   
// expected-warning{{string literal concatenation of 
predefined identifier 'L__FUNCSIG__' is a Microsoft extension}}
+}
+

RIscRIpt wrote:
> tahonermann wrote:
> > How about testing some other encoding prefix combinations?
> >   u"" __FUNCTION // Ok; UTF-16 string literal
> >   u"" L__FUNCTION__ // ill-formed.
> > 
> Good idea. I am not sure whether I should specify `-std` in the test command. 
> I'll leave it without, because current default standard is C++17, and I 
> believe it's not going to be decreased.
> 
> And I think there are enough tests of checking values of these macros. So, in 
> tests for encoding I am going to check types.
Thank you for adding those. I think we should check values for a couple of 
cases as well. Consider a function name that contains `𐀀` (U+1 LINEAR B 
SYLLABLE B008 A). In UTF-8, that character requires four code units (F0 90 80 
80) while in UTF-16 it requires two (D800 DC00). We'll want to ensure that such 
characters are properly converted between encodings.
  void test_encoding𐀀() {
ASSERT_EQ(u"" __FUNCTION__, u"test_encoding𐀀");
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914

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

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9
+// Note: We must use the real path here, because the logic to detect case
+// mismatch relies on resolving the real path and checking that casing differs.
+// If we use %t and we are on a substitute drive S: mapping to C:\subst,
+// then we will compare "S:\test.dir\FOO.h" to "C:\subst\test.dir\foo.h"
+// and avoid emitting the diagnostic because the structure is different.

Hmm, is it really desirable or necessary that the case mismatch logic resolve 
substituted drives? I wouldn't think so. This seems like another case where our 
common canonicalization would be desired.


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

https://reviews.llvm.org/D154130

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

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

I think this is looking good. The only big thing I noticed is some code that 
looks like it should have been removed with the last set of changes. Otherwise, 
just some minor concerns and suggestions.




Comment at: clang/lib/Parse/ParseExpr.cpp:1299-1301
+Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
+ConsumeToken();
+break;

I believe this code should be removed now; it is no longer reachable.



Comment at: clang/lib/Parse/ParseExpr.cpp:1309-1311
+if (!(getLangOpts().MicrosoftExt &&
+  TokenIsLikeStringLiteral(Tok, getLangOpts()) &&
+  TokenIsLikeStringLiteral(NextToken(), getLangOpts( {

I think this is good, but how about a comment to make the intent more clear?



Comment at: clang/lib/Sema/SemaExpr.cpp:1903
+  case tok::kw___PRETTY_FUNCTION__:
+return PredefinedExpr::PrettyFunction;
+  }





Comment at: clang/lib/Sema/SemaExpr.cpp:2010-2013
+OS << "R\"EFLPM("
+   << PredefinedExpr::ComputeName(getPredefinedExprKind(Tok.getKind()),
+  currentDecl)
+   << ")EFLPM\"";

"EFLPM"? I'm assuming this is an attempt to avoid an accidental clash with the 
computed name. Does this suffice to ensure a clash can't happen? If not, can we 
do better? Per http://eel.is/c++draft/lex.string#nt:r-char and 
http://eel.is/c++draft/lex.charset#1, we have a lot of flexibility regarding 
which characters to use.



Comment at: clang/test/Sema/ms_predefined_expr.cpp:115
+   
// expected-warning{{string literal concatenation of 
predefined identifier 'L__FUNCSIG__' is a Microsoft extension}}
+}
+

How about testing some other encoding prefix combinations?
  u"" __FUNCTION // Ok; UTF-16 string literal
  u"" L__FUNCTION__ // ill-formed.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914

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

[PATCH] D155846: [NFC][clang] Fix static analyzer concerns

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> I agree. I'll try to isolate a reproducible test case and report it to the 
> vendor.

Done.

I'm ambivalent with regard to keeping or discarding the proposed change, but 
the static analysis issue should be triaged as a false positive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155846

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

[PATCH] D155776: [NFC] Add checks for self-assignment.

[Tom Honermann via Phabricator via cfe-commits]

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

On second thought, and after having read 
https://stackoverflow.com/a/9322542/11634221, I think this change is fine. 
There are legitimate scenarios for which self move assignment should work in 
the sense that the object value is preserved or is left in a valid but 
unspecified state; UB should be avoided.


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

https://reviews.llvm.org/D155776

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

[PATCH] D155842: [NFC][clang] Fix static analyzer concerns

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

This is a good change since the default copy constructor would do bad things 
when `ownsOutputFile` is true. The copy assignment operator is already 
implicitly deleted because of the presence of a data member of reference type, 
but I think it is helpful to explicitly delete both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155842

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

[PATCH] D155846: [NFC][clang] Fix static analyzer concerns

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> there's no bug being fixed which makes me think this should go back to the 
> static analysis vendor to report this as a false positive.

I agree. I'll try to isolate a reproducible test case and report it to the 
vendor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155846

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

[PATCH] D155849: [NFC][clang] Fix static analyzer concerns

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

Looks good; the default implementations would definitely do the wrong thing, so 
this is a good find. My suggested edit is just to add a blank line.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1823-1826
+  OMPTransformDirectiveScopeRAII(const OMPTransformDirectiveScopeRAII &) =
+  delete;
+  OMPTransformDirectiveScopeRAII &
+  operator=(const OMPTransformDirectiveScopeRAII &) = delete;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155849

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

[PATCH] D155776: [NFC] Add checks for self-assignment.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

Hmm, it seems exceptionally unlikely for a move assignment operator to be 
invoked on an object with itself as the source argument. That would require an 
explicit use of `std::move()` as in:

  Value x = ...;
  x = std::move(x);

Rather than protecting against that and making it a no-opt, I think we would 
want to be informed of code that manages to do so because it is probably not 
intended. We could instead `assert(this !=RHS)`, but I'm not sure that is 
warranted either.

I'm assuming this patch was made in response to a static analysis report. If 
so, I suggest we follow up with the static analysis vendor.


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

https://reviews.llvm.org/D155776

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

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

It is unclear to me why/when we would ever want the substitute drive expansion; 
the modified tests aren't very elucidating. My naive expectation is that we 
effectively want to restore the Python 3.7 behavior.




Comment at: clang/test/ExtractAPI/relative_include.m:24
+// RUN: %{/t:real}/Frameworks/MyFramework.framework/Headers/MyHeader.h \
+// RUN: %{/t:real}/QuotedHeader.h \
 // RUN: -o %t/output.json 2>&1 -verify | FileCheck -allow-empty %s

I'm curious why this test requires the `%{/t:real}` normalization. Why would it 
be desirable to expand substitute drives for this test?



Comment at: clang/test/Lexer/case-insensitive-include-win.c:8
 // RUN: touch %t.dir/foo.h
-// RUN: not %clang_cl /FI\\?\%t.dir\FOO.h /WX -fsyntax-only %s 2>&1 | 
FileCheck %s
+// RUN: not %clang_cl /FI\\?\%{t:real}.dir\FOO.h /WX -fsyntax-only %s 2>&1 | 
FileCheck %s
 

I'm curious why this test requires the `%{/t:real}` normalization. Why would it 
be desirable to expand substitute drives for this test?


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

https://reviews.llvm.org/D154130

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/include/clang/Basic/TokenKinds.h:87-93
+/// Return true if this token is a predefined macro
+/// unexpandable by MSVC preprocessor.
+inline bool isUnexpandableMsMacro(TokenKind K) {
+  return K == tok::kw___FUNCTION__ || K == tok::kw___FUNCSIG__ ||
+ K == tok::kw_L__FUNCTION__ || K == tok::kw_L__FUNCSIG__ ||
+ K == tok::kw___FUNCDNAME__;
+}

cor3ntin wrote:
> tahonermann wrote:
> > RIscRIpt wrote:
> > > tahonermann wrote:
> > > > 
> > > Thanks, I like the name. But shouldn't we reflect that we are referring 
> > > to only Microsoft (unexpandable) macros? How about 
> > > `isFunctionLocalPredefinedMsMacro`?
> > I don't think the Microsoft association is meaningful. Sure, some of the 
> > predefined macros are only treated differently when compiling in Microsoft 
> > mode, but some of those macros aren't Microsoft specific. Also, there might 
> > be macros provided by other implementations that we'll want to emulate some 
> > day.
> I think it is, there is currently no way to link 
> `isFunctionLocalPredefinedMacro` to the MS feature. "MSPredefinedMacro" is 
> pretty self explanatory, same reason we most often - but not always - use GNU 
> in the name of function related to GNU extensions.
> There are enough similar-sounding features and extensions that we should try 
> to make it clear which feature we are talking about.
Perhaps we still haven't found the right name then. With the name that I've 
been advocating, this function should also return true for 
`tok::kw___PRETTY_FUNCTION__` even though that macro should not expand to 
something that can be concatenated with other string literals (whether 
compiling in Microsoft mode or not).

The Microsoft extension is the localized expansion to a string literal that can 
be concatenated with other string literals. We probably should isolate the 
conditions for that behavior to one place and if we do that, then I guess 
naming this function as specific to Microsoft mode is ok; we can always rename 
it later if it gets used for non-Microsoft extensions.

Here is my suggestion then:
  // Return true if the token corresponds to a function local predefined
  // macro that, in Microsoft modes, expands to a string literal (that can
  // then be concatenated with other string literals).
  inline bool isMsFunctionLocalStringLiteralMacro(TokenKind K, const 
LangOptions &langOpts) {
return langOpts.MicrosoftExt && (
K == tok::kw___FUNCTION__ || K == tok::kw_L__FUNCTION__ ||
K == tok::kw___FUNCSIG__ || K == tok::kw_L__FUNCSIG__ ||
K == tok::kw___FUNCDNAME__);
  }  

This will avoid the need for callers to check for `MicrosoftExt`; that is what 
I really want to avoid since it is easy to forget to do that check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Basic/TokenKinds.h:87-93
+/// Return true if this token is a predefined macro
+/// unexpandable by MSVC preprocessor.
+inline bool isUnexpandableMsMacro(TokenKind K) {
+  return K == tok::kw___FUNCTION__ || K == tok::kw___FUNCSIG__ ||
+ K == tok::kw_L__FUNCTION__ || K == tok::kw_L__FUNCSIG__ ||
+ K == tok::kw___FUNCDNAME__;
+}

RIscRIpt wrote:
> tahonermann wrote:
> > 
> Thanks, I like the name. But shouldn't we reflect that we are referring to 
> only Microsoft (unexpandable) macros? How about 
> `isFunctionLocalPredefinedMsMacro`?
I don't think the Microsoft association is meaningful. Sure, some of the 
predefined macros are only treated differently when compiling in Microsoft 
mode, but some of those macros aren't Microsoft specific. Also, there might be 
macros provided by other implementations that we'll want to emulate some day.



Comment at: clang/include/clang/Sema/Sema.h:5681
 
+  std::vector ExpandUnexpandableMsMacros(ArrayRef Toks);
   ExprResult BuildPredefinedExpr(SourceLocation Loc,

RIscRIpt wrote:
> tahonermann wrote:
> > 
> Renamed to `ExpandFunctionLocalPredefinedMsMacros`. If you think my addition 
> of `Ms` is redundant, let me know.
I don't think the `Ms` is redundant, but I do think it is unnecessary and 
slightly confusing (`__FUNCTION__`) is not a Microsoft macro.



Comment at: clang/lib/Parse/ParseExpr.cpp:1310-1311
+  case tok::kw_L__FUNCSIG__:  // primary-expression: L__FUNCSIG__ [MS]
+if (!getLangOpts().MicrosoftExt ||
+!TokenIsLikeStringLiteral(NextToken(), getLangOpts())) {
+  Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);

`TokenIsLikeStringLiteral()` checks `MicrosoftExt`, so the check here is 
redundant. This is an example of why I would like to see the `MicrosoftExt` 
checking pushed down to `isFunctionLocalPredefinedMsMacro()`; that really seems 
where that checking belongs to me.



Comment at: clang/lib/Parse/ParseExpr.cpp:3298-3299
+  assert(
+  (StringToks.size() != 1 ||
+   !tok::isFunctionLocalPredefinedMsMacro(StringToks[0].getKind())) &&
+  "single predefined identifiers shall be handled by ActOnPredefinedExpr");

Is there a missing check for `MicrosoftExt` here? This is another example of 
why I would prefer to see those checks pushed down to 
`isFunctionLocalPredefinedMsMacro()`.



Comment at: clang/lib/Sema/SemaExpr.cpp:1979-1980
+  Decl *currentDecl = getCurLocalScopeDecl();
+  if (!currentDecl)
+currentDecl = Context.getTranslationUnitDecl();
+  std::vector ExpandedToks;

This could use a comment since it isn't obvious why the translation unit 
declaration is used when not in a local scope declaration of some kind.



Comment at: clang/lib/Sema/SemaExpr.cpp:2001-2004
+OS << '"'
+   << Lexer::Stringify(PredefinedExpr::ComputeName(
+  getPredefinedExprKind(Tok.getKind()), currentDecl))
+   << '"';

RIscRIpt wrote:
> tahonermann wrote:
> > A diagnostic is issued if the call to `getCurScopeDecl()` returned a 
> > translation unit declaration (at least in Microsoft mode). Does it make 
> > sense to pass a pointer to a `TranslationUnitDecl` here?
> Shortly, yes, kind of. `ComputeName` can handle `TranslationUnitDecl` in 
> which case it returns an empty string. This way we implicitly (without 
> additional code) create empty string-literal Tokens which can be handled in 
> `StringLiteralParser`. And we cannot pass non-string-literal tokens into 
> `StringLiteralParser`.
Ah, ok. And I see it even differentiates what name is produced for 
`__PRETTY_FUNCTION__`. That leaves me wondering what the right behavior should 
be at class and namespace scope, but maybe I'm better off not asking questions 
I don't want to know the answer to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153914

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

[PATCH] D155610: [Clang][ExprConstant] Print integer instead of character on static assertion failure

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/test/Lexer/cxx1z-trigraphs.cpp:24
 // expected-error@11 {{}} expected-warning@11 {{trigraph ignored}}
-// expected-error@13 {{failed}} expected-warning@13 {{trigraph ignored}} 
expected-note@13 {{evaluates to ''?' == '#''}}
+// expected-error@13 {{failed}} expected-warning@13 {{trigraph ignored}} 
expected-note@13 {{evaluates to '63 == 35'}}
 // expected-error@16 {{}}

aaron.ballman wrote:
> I think the original diagnostic was actually more understandable as it 
> relates more closely to what's written in the static assertion. I could 
> imagine something like `evaluates to '?' (63) == '#' (35)` would also be 
> reasonable.
I agree. I would also be ok with printing the integer value as primary with the 
character as secondary:
  evaluates to 63 ('?') == 35 ('#')

There are two kinds of non-printable characters:
  # Control characters (including new-line)
  # character values that don't correspond to a character (e.g., lone trailing 
characters or invalid code unit values).
For the first case, I would support printing them as either C escapes or 
universal-character-names. e.g.,
  evaluates to 0 ('\0') == 1 (\u0001)
For the second case, I would support printing them as C hex escapes. e.g, 
  evaluates to -128 ('\x80') == -123 ('\x85')




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155610

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

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

>> In D154130#4486898 , @tahonermann 
>> wrote:
>>
>>> 95% of the %>t are around clang modulemap files, because that code resolves 
>>> real paths in C++ by design, so I can't avoid it.
>>
>> Can you link to evidence that this behavior is by design? It isn't obvious 
>> to me why modulemap files would demand different behavior; especially since 
>> that would exacerbate MAX_PATH problems.
>
> I agree with you. The original rationale seems to be here:
> https://github.com/llvm/llvm-project/blob/926f3759ec62a8f170e76a60316cc0bdd9dd2ec9/clang/lib/Lex/HeaderSearch.cpp#L257

Thank you for finding that. I took a look at the code and it looks to me like 
it would be safe to change ModuleMap::canonicalizeModuleMapPath() 

 to  use a drive preserving canonicalization. I think the worst case impact 
would be that a module cache lookup would fail thereby leading to a cache miss 
and rebuild of the target module. That could impose a significant performance 
and drive space penalty in the event that substitute drive paths are changed, 
but I would expect such changes to be rare.


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

https://reviews.llvm.org/D154130

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

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:118-120
+def ext_concat_predef_ms : ExtWarn<
+  "concatenation of predefined identifier '%0' is a Microsoft extension">,
+  InGroup;

I think it makes since to be more explicit that the concatenation is with 
regard to string literals.



Comment at: clang/include/clang/Basic/TokenKinds.h:87-93
+/// Return true if this token is a predefined macro
+/// unexpandable by MSVC preprocessor.
+inline bool isUnexpandableMsMacro(TokenKind K) {
+  return K == tok::kw___FUNCTION__ || K == tok::kw___FUNCSIG__ ||
+ K == tok::kw_L__FUNCTION__ || K == tok::kw_L__FUNCSIG__ ||
+ K == tok::kw___FUNCDNAME__;
+}





Comment at: clang/include/clang/Parse/Parser.h:578-582
+  bool isTokenConcatenable() const {
+return isTokenStringLiteral() ||
+   getLangOpts().MicrosoftExt &&
+   tok::isMSPredefinedMacro(Tok.getKind());
+  }

RIscRIpt wrote:
> cor3ntin wrote:
> > Unless you find a better name,  I think it's preferable to keep 
> > `isTokenConcatenable` and `isTokenPredefinedMsMacro` as separate functions.
> > Also, this seems like a weird place to check for  
> > `getLangOpts().MicrosoftExt`
> Regarding `getLangOpts().MicrosoftExt`. If you are talking about it's 
> presence in a function which name is meant to be used as a predicate, I 
> agree. If you are talking about `class Parser`, then there're other places 
> with references to `getLangOpts()`.
> 
> Without such function `ParseStringLiteralExpression` implementation would be 
> too verbose.
> Let's decide what we can do after I address other comments.
> 
> Meanwhile, I renamed it to `isTokenLikeStringLiteral()`.
I suggest passing `getLangOpts()` to `isFunctionLocalPredefinedMacro` 
(`isUnexpandableMsMacro`) and letting it determine whether the token is or is 
not one of these special not-really-a-string-literal macro things. This will 
facilitate additional such macros controlled by different options while also 
colocating the option check with the related tokens.



Comment at: clang/include/clang/Sema/Sema.h:5681
 
+  std::vector ExpandUnexpandableMsMacros(ArrayRef Toks);
   ExprResult BuildPredefinedExpr(SourceLocation Loc,





Comment at: clang/lib/Parse/ParseExpr.cpp:1300-1307
+if (!getLangOpts().MicrosoftExt || !tok::isUnexpandableMsMacro(SavedKind) 
||
+!tok::isUnexpandableMsMacro(NextToken().getKind()) &&
+!tok::isStringLiteral(NextToken().getKind())) {
+  Res = Actions.ActOnPredefinedExpr(Tok.getLocation(), SavedKind);
+  ConsumeToken();
+  break;
+}

Since the conditional code is only relevant for some of the tokens here, I 
would prefer to see the other token kinds (`kw___func__`, 
`kw___PRETTY_FUNCTION__`) handled separately to avoid the conditional 
fallthrough. That means duplicating the calls to 
`Actions.ActOnPredefinedExpr()` and `ConsumeToken()` between the blocks, but 
that bothers me less than having to understand the complicated condition.



Comment at: clang/lib/Parse/ParseExpr.cpp:3277-3280
+  // String concatenation.
+  // Note that keywords like __func__ and __FUNCTION__
+  // are not considered to be strings for concatenation purposes,
+  // unless Microsoft extensions are enabled.

I think `__func__` is not considered a string literal in Microsoft mode, so I 
think this comment needs some adjusting.



Comment at: clang/lib/Parse/ParseExpr.cpp:3288-3291
+  assert(
+  (StringToks.size() != 1 ||
+   !tok::isUnexpandableMsMacro(StringToks[0].getKind())) &&
+  "single predefined identifiers shall be handled by ActOnPredefinedExpr");

What is the reason for requiring non-concatenated predefined function local 
macros to be handled by `ActOnPredefinedExpr()`?



Comment at: clang/lib/Sema/Sema.cpp:1494
 
+Decl *Sema::getCurScopeDecl() {
+  if (const BlockScopeInfo *BSI = getCurBlock())

`getCurScopeDecl` isn't a great name since this doesn't handle class or 
namespace scopes. How about `getCurLocalScopeDecl`? That name isn't quite right 
either since this can return a `TranslationUnitDecl`, but I think it matches 
the intent fairly well.

None of the callers need the actual `TranslationUnitDecl` that is returned. I 
think this function can return `nullptr` is the current scope isn't function 
local and callers can issue the relevant diagnostic in that case (thus avoiding 
the need to check that a translation unit decl was returned).



Comment at: clang/lib/Sema/SemaExpr.cpp:1971-1974
+  // MSVC treats some of predefined identifiers (e.g. __FUNCTION__) as
+  // expandable macros defined as string lite

[PATCH] D153621: [Clang] Correctly handle $, @, and ` when represented as UCN

[Tom Honermann via Phabricator via cfe-commits]

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

> I'll probably merge that EoW unless you scream!

Why wait? :) Looks good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153621

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

[PATCH] D153892: [NFC] Initialize class member pointers to nullptr.

[Tom Honermann via Phabricator via cfe-commits]

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

Thanks, Sindhu! This looks good to me!


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

https://reviews.llvm.org/D153892

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

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> 95% of the %>t are around clang modulemap files, because that code resolves 
> real paths in C++ by design, so I can't avoid it.

Can you link to evidence that this behavior is by design? It isn't obvious to 
me why modulemap files would demand different behavior; especially since that 
would exacerbate `MAX_PATH` problems.

I'm not fond of the `safe_abs_path` name; "safe" doesn't communicate anything 
and the implementation is no more safe than `os.path.abspath` or 
`os.path.realpath`. Suggested alternatives:

- `short_abs_path`
- `shortest_abs_path`
- `abs_path_no_subst_drive`
- `abs_path_preserve_drive`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

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

[PATCH] D153418: Adding iconv support to CharSetConverter class

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a subscriber: hubert.reinterpretcast.
tahonermann added a comment.

> Please correct me if I'm wrong, I'm not too familiar with icu4c, but I think 
> adding support for ICU would be the better long-term solution since it seems 
> to allow the same behaviour across different platforms.

I tend to agree. Additionally, as more Unicode support is added to C++, the 
likelihood that we'll want to use other functionality from ICU increases.

> However, the issue on the z/OS platform is that there currently isn't support 
> for this library so iconv seems to be the only solution we can use until we 
> do get support. So would an alternative be to use iconv only on z/OS (and 
> hopefully this is a temporary solution until icu is supported on z/OS) and 
> use icu on all other platforms?

ICU isn't supported on z/OS because the historical z/OS compiler (xlC) never 
gained support for C++11 or later so support for z/OS was dropped when ICU 
moved to C++11. Now that IBM has embraced LLVM and Clang, I would expect it to 
be possible to build ICU for z/OS again with moderate porting effort. It would 
be great if someone from IBM could confirm whether such an effort is underway 
(@hubert.reinterpretcast?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153418

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

[PATCH] D153621: [Clang] Correctly handle $, @, and ` when represented as UCN

[Tom Honermann via Phabricator via cfe-commits]

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

Changes look good; I added a number of suggested edits for minor issues.




Comment at: clang/docs/ReleaseNotes.rst:203-204
 
+- Implemented `WG14 N3124 
_`,
+  which allow any universal character name to appear in string literals.
+





Comment at: clang/docs/ReleaseNotes.rst:530-531
   (`#38717 _`).
+- Fix an assertion when using ``\u0024`` as an identifier, by disallowing
+  that construct (`#62133 
_`).
 





Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:204-207
+def warn_c2x_compat_literal_ucn_control_character : Warning<
+  "universal character name referring to a control character "
+  "incompatible with C standards before C2x">,
+  InGroup, DefaultIgnore;





Comment at: clang/lib/Lex/Lexer.cpp:3487-3492
+  // C2x 6.4.3p2: A universal character name shall not designate a code point
+  //   where the hexadecimal value is in the range D800 through DFFF inclusive
+  //   or greater than 1080). A universal-character-name outside the
+  //   c-char-sequence of a character constant, or the s-char-sequence
+  //   of a string-literal shall not designate a control character
+  //   or a character in the basic character.  
  set.





Comment at: clang/lib/Lex/LiteralSupport.cpp:617-618
 
   // C++11 allows UCNs that refer to control characters and basic source
   // characters inside character and string literals
   if (UcnVal < 0xa0 &&





Comment at: clang/lib/Lex/LiteralSupport.cpp:630
+   diag::warn_cxx98_compat_literal_ucn_escape_basic_scs
+ : diag::warn_c2x_compat_literal_ucn_escape_basic_scs )
 << StringRef(&BasicSCSChar, 1);





Comment at: clang/test/Preprocessor/ucn-allowed-chars.c:16
 
-
 // Identifier initial characters

I assume this line was deleted to minimize the disruption to line numbers due 
to the additional RUN line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153621

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

[PATCH] D153892: [NFC] Initialize class member pointers to nullptr.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added inline comments.
This revision now requires changes to proceed.
Herald added a subscriber: wangpc.



Comment at: clang/lib/Frontend/ASTConsumers.cpp:192-198
+  bool HandleTopLevelDecl(DeclGroupRef D) override {
   for (DeclGroupRef::iterator I = D.begin(), E = D.end(); I != E; ++I)
 HandleTopLevelSingleDecl(*I);
   return true;
 }
 
 void HandleTopLevelSingleDecl(Decl *D);

Indentation is still inconsistent here.

I suggest following the indentation used for `StackAddrEscapeChecker` in 
`clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp`; Don't indent 
the `ASTViewer` class definition within the unnamed namespace and do indent the 
class members.


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

https://reviews.llvm.org/D153892

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

[PATCH] D153926: [NFC] Initialize class member pointers to nullptr.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.
Herald added a subscriber: wangpc.

These changes look find to me.


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

https://reviews.llvm.org/D153926

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

[PATCH] D153589: [NFC] Initialize pointer fields and remove a needless null check.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

These changes look fine to me.


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

https://reviews.llvm.org/D153589

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

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

[Tom Honermann via Phabricator via cfe-commits]

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

Sorry for the long delay reviewing again. I noted some mostly minor issues. I'm 
on vacation next week, but I'll try to watch for updates.

I'm still concerned that the changes to the code that was already performing 
rank comparisons don't explicitly do anything for unordered cases. I think I 
would feel better if we at least asserted that the comparisons are not 
unordered.

I'd like to see tests added to exercise `va_arg` and passing the extended types 
through `...`; this will exercise the promotion related changes.




Comment at: clang/lib/AST/ASTContext.cpp:191
+   FloatingRankCompareResult::FRCR_Greater,
+   FloatingRankCompareResult::FRCR_Equal;
+

I just realized that none of these comparisons results in 
`FRCR_Equal_Lesser_Subrank` or `FRCR_Equal_Greater_Subrank`. I guess those 
won't actually be used until support for `std::float32_t` and `std::float64_t` 
are added. That means that we don't have a way to test code that performs 
subrank checks though.



Comment at: clang/lib/AST/ASTContext.cpp:7141-7145
+/// If LHS > RHS, return FRCR_Greater.  If LHS == RHS, return FRCR_Equal. If
+/// LHS < RHS, return FRCR_Lesser. If the values representedable by the two
+/// are not subset of each other, return FRCR_Unordered. If LHS == RHS but
+/// LHS has a higher subrank than RHS return FRCR_Equal_Greater_Subrank else
+/// return FRCR_Equal_Lesser_Subrank.





Comment at: clang/lib/Frontend/InitPreprocessor.cpp:461
+  Builder.defineMacro("__STDCPP_FLOAT16_T__", "1");
+  Builder.defineMacro("__STDCPP_BFLOAT16_T__", "1");
+}

Should the definition of `__STDCPP_BFLOAT16_T__` be conditional on something 
like `Ctx.getTargetInfo().hasFullBFloat16Type()`?



Comment at: clang/lib/Sema/SemaCast.cpp:1367
+  Self.Context.doCXX23ExtendedFpTypesRulesApply(DestType, SrcType)) {
+// Support for cast between fp16 and bf16 doesn't exist yet.
+if (!((DestType->isBFloat16Type() || DestType->isFloat16Type()) &&

codemzs wrote:
> tahonermann wrote:
> > Should this be a FIXME comment? What happens in this case? Should we 
> > perhaps assert on such attempted conversions?
> I have added the FIXME. There is a test case for this scenario:
> 
> ```_Float16 f16_val_7 = static_cast<_Float16>(1.0bf16); // expected-error 
> {{static_cast from '__bf16' to '_Float16' is not allowed}}```
> 
> If you still believe it might be better to add an assert I can do that, 
> please let me know.
If the condition can be legitimately hit, then we definitely want error 
handling rather than an assert. Thanks, this looks good.



Comment at: clang/lib/Sema/SemaChecking.cpp:10451
   return ICE->getCastKind() == CK_FloatingCast &&
- S.Context.getFloatingTypeOrder(From, To) < 0;
+ S.Context.getFloatingTypeOrder(From, To) == FRCR_Lesser;
 }

I'm not sure this is right. If I understand correctly, the C++23 extended FP 
types don't participate in argument promotions. Perhaps they should be excluded 
here.



Comment at: clang/lib/Sema/SemaChecking.cpp:14170-14195
   int Order = S.getASTContext().getFloatingTypeSemanticOrder(
   QualType(SourceBT, 0), QualType(TargetBT, 0));
-  if (Order > 0) {
+  if (Order == FRCR_Greater) {
 // Don't warn about float constants that are precisely
 // representable in the target type.
 Expr::EvalResult result;
 if (E->EvaluateAsRValue(result, S.Context)) {

Should `FRCR_Unordered` be handled in some way here? It seems like we should at 
least assert that the types are not unordered.



Comment at: clang/lib/Sema/SemaExpr.cpp:1244-1246
+assert(((order != FRCR_Equal) &&
+(order == FRCR_Lesser || order == FRCR_Equal_Lesser_Subrank)) &&
+   "illegal float comparison");

Since the assertion requires `order` to be one of `FRCR_Lesser` or 
`FRCE_Equal_Lesser_Subrank`, there is no need to check for `FRCR_Equal`.



Comment at: clang/lib/Sema/SemaExpr.cpp:1235-1237
+if (order == FRCR_Unordered) {
+  return QualType();
+}

codemzs wrote:
> tahonermann wrote:
> > Return of an invalid type is a new behavior for this function and it isn't 
> > clear to me that callers will handle it (or be expected to handle it) such 
> > that a diagnostic will be generated. Perhaps a diagnostic should be issued 
> > here? Or perhaps this should be an assertion?
> It results in an error, please see the below test case:
> 
> ```auto f16_bf16 = 1.0f16 + 1.0bf16; // expected-error {{invalid operands to 
> binary expression ('_Float16' and '__bf16')}}```
> 
> This function is only called by `UsualArithmeticConversions` which returns 
>

[PATCH] D151606: [NFC][CLANG] Fix Static Code Analyzer Concerns with bad bit right shift operation in getNVPTXLaneID()

[Tom Honermann via Phabricator via cfe-commits]

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

The static analysis results look suspect since there does not appear to be a 
way for `Log2_32` to return a value larger than 31. Regardless, the change 
looks safe to me; `GV_Warp_Size` should always be greater than 0, so `Log2_32` 
should always return a non-negative number that is less than 32.


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

https://reviews.llvm.org/D151606

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

[PATCH] D150931: [NFC][Clang] Fix Static Code Analysis Concerns with copy without assign

[Tom Honermann via Phabricator via cfe-commits]

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

These changes look fine to me.


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

https://reviews.llvm.org/D150931

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

[PATCH] D150744: [NFC][CLANG] Fix uninitialized scalar field issues

[Tom Honermann via Phabricator via cfe-commits]

tahonermann accepted this revision.
tahonermann added a comment.

> is it ok to land this patch?

No objection from me!


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

https://reviews.llvm.org/D150744

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

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

[Tom Honermann via Phabricator via cfe-commits]

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

> They are NFC, it's just not 100% only a refactoring, since it adds the new 
> ASCII-only case.

Ok, thanks. Changes look good. I noticed one comment that appears to be 
incorrect; please just remove it when committing the changes.




Comment at: clang/lib/Frontend/TextDiagnostic.cpp:128
+
+  // We now know that the next character is a multi-byte character.
+  // Convert it to UTF32 and check if it's printable.

This comment is not correct; `Begin` might still point to a non-printable 
character with a code point value less than 0x80. I suggest just removing the 
comment.


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

https://reviews.llvm.org/D150843

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

[PATCH] D128059: [Clang] Add a warning on invalid UTF-8 in comments.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

@cor3ntin, sorry for failing to keep up with reviews; I know this has already 
been committed. I did spot a couple of typos should you feel inclined to 
address them.




Comment at: clang/lib/Lex/Lexer.cpp:2695
+  // diagnostic only once per entire ill-formed subsequence to avoid
+  // emiting to many diagnostics (see http://unicode.org/review/pr-121.html).
+  bool UnicodeDecodingAlreadyDiagnosed = false;





Comment at: clang/lib/Lex/Lexer.cpp:2398
+  // diagnostic only once per entire ill-formed subsequence to avoid
+  // emiting to many diagnostics (see http://unicode.org/review/pr-121.html).
+  bool UnicodeDecodingAlreadyDiagnosed = false;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128059

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

[PATCH] D146924: [clang] Add support for dollar sign in ud_suffix of numeric constants

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> I wrote a paper https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3124.pdf

It appears you back ported that paper from the future: "Date: 2024-22-04" :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146924

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

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

The changes look good to me. I suggested two minor edits to align with 
parameter name changes.

The summary states that the changes are not quite NFC. In that case, we would 
ideally have a test that demonstrates the changed behavior. Would adding such a 
test be challenging?




Comment at: clang/lib/Frontend/TextDiagnostic.cpp:94
 /// \param SourceLine The line of source
 /// \param i Pointer to byte index,
 /// \param TabStop used to expand tabs





Comment at: clang/lib/Frontend/TextDiagnostic.cpp:101
   unsigned TabStop) {
-  assert(i && "i must not be null");
-  assert(*ihttps://reviews.llvm.org/D150843/new/

https://reviews.llvm.org/D150843

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

[PATCH] D150913: [Clang][BFloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

I'm late to review and can no longer stamp an approval on this, but I'll note 
for the historical record that the changes look good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150913

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

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

This is looking good to me. I added a few comments seeking clarification.

I'm concerned about the changes to the existing calls to 
`getFloatingTypeOrder()`. The changes look like they will preserve prior 
behavior for standard types just fine, but I'm not sure whether they implement 
the right behavior for the extended types. Defining comparison functions that 
are explicit in their handling of `FRCR_Unordered` and subrank comparisons 
would help me feel more confident about the changes, the intent, and my ability 
to audit for the desired behavior.




Comment at: clang/include/clang/Lex/LiteralSupport.h:75
   bool isBitInt : 1;// 1wb, 1uwb (C2x)
+  bool isBF16 : 1;  // 1.0bf
   uint8_t MicrosoftInteger; // Microsoft suffix extension i8, i16, i32, or i64.

codemzs wrote:
> tahonermann wrote:
> > Is this for `__bf16` or for `std::bfloat16_t`?
> Its for `std::bfloat16_t`, I don't believe `__bf16` has a literal suffix. 
I guess it does now! :)



Comment at: clang/lib/AST/ASTContext.cpp:126-127
+return 4;
+  default:
+llvm_unreachable("Not a CXX23+ floating point builtin type");
+  }

Assuming there is no need to handle `__float128` and `__ibm128` here, how about 
a comment to state why they don't require support here?



Comment at: clang/lib/Sema/Sema.cpp:706
+auto conversionRank = Context.getFloatingTypeOrder(TypeTy, ExprTy);
+if (conversionRank < FRCR_Greater) {
+  Diag(E->getExprLoc(),

This comparison depends on the order of the enumerators in 
`FloatingRankCompareResult` and that seems brittle. I suggest either 
encompassing such comparisons in a function or, at least, adding a comment to 
the enumeration definition that explains the ordering requirements.



Comment at: clang/lib/Sema/SemaCast.cpp:1367
+  Self.Context.doCXX23ExtendedFpTypesRulesApply(DestType, SrcType)) {
+// Support for cast between fp16 and bf16 doesn't exist yet.
+if (!((DestType->isBFloat16Type() || DestType->isFloat16Type()) &&

Should this be a FIXME comment? What happens in this case? Should we perhaps 
assert on such attempted conversions?



Comment at: clang/lib/Sema/SemaExpr.cpp:1235-1237
+if (order == FRCR_Unordered) {
+  return QualType();
+}

Return of an invalid type is a new behavior for this function and it isn't 
clear to me that callers will handle it (or be expected to handle it) such that 
a diagnostic will be generated. Perhaps a diagnostic should be issued here? Or 
perhaps this should be an assertion?



Comment at: clang/lib/Sema/SemaOverload.cpp:4202
+  if (S.Context.getFloatingTypeOrder(SCS2.getToType(1),
+ SCS1.getFromType()) < FRCR_Equal) 
{
+return ImplicitConversionSequence::Better;

This comparison includes `FRCR_Unordered`, is that intended? (another case at 
line 4225 below)


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

https://reviews.llvm.org/D149573

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

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> @tahonermann Just a quick note - I've made the changes you initially 
> suggested in the RFC. When you get a chance, could you have another look?

Yes, I hope to today. If not, I'll make it a priority for tomorrow.


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

https://reviews.llvm.org/D149573

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

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

[Tom Honermann via Phabricator via cfe-commits]

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

Thanks @Manna, this looks good to me now!




Comment at: clang/include/clang/Sema/ParsedAttr.h:705-707
+  // The move assignment operator is defined as deleted pending further
+  // motivation.
+  AttributePool &operator=(AttributePool &&pool) = delete;

Manna wrote:
> tahonermann wrote:
> > Let's explicitly delete the copy assignment operator too since the copy 
> > constructor is deleted.
> @tahonermann , copy assignment operator has already been explicitly deleted 
> at line no 701. I made this change part of https://reviews.llvm.org/D149718. 
> It was a rebase issue, so it did show it before with my previous patch. I 
> have fixed it now and added comment since the copy assignment was added 
> explicitly as deleted.
Ah, perfect!


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

https://reviews.llvm.org/D150411

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

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

[Tom Honermann via Phabricator via cfe-commits]

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

Splitting this into two patches (one to do the renames, another to perform the 
other changes) would simplify review.




Comment at: clang/lib/Frontend/TextDiagnostic.cpp:118-119
 
-  unsigned char const *begin, *end;
-  begin = reinterpret_cast(&*(SourceLine.begin() + *i));
-  end = begin + (SourceLine.size() - *i);
-
-  if (llvm::isLegalUTF8Sequence(begin, end)) {
-llvm::UTF32 c;
-llvm::UTF32 *cptr = &c;
-unsigned char const *original_begin = begin;
-unsigned char const *cp_end =
-begin + llvm::getNumBytesForUTF8(SourceLine[*i]);
-
-llvm::ConversionResult res = llvm::ConvertUTF8toUTF32(
-&begin, cp_end, &cptr, cptr + 1, llvm::strictConversion);
-(void)res;
-assert(llvm::conversionOK == res);
-assert(0 < begin-original_begin
-   && "we must be further along in the string now");
-*i += begin-original_begin;
-
-if (!llvm::sys::locale::isPrint(c)) {
-  // If next character is valid UTF-8, but not printable
-  SmallString<16> expandedCP("");
-  while (c) {
-expandedCP.insert(expandedCP.begin()+3, llvm::hexdigit(c%16));
-c/=16;
-  }
-  while (expandedCP.size() < 8)
-expandedCP.insert(expandedCP.begin()+3, llvm::hexdigit(0));
-  return std::make_pair(expandedCP, false);
-}
+  const unsigned char *Begin =
+  reinterpret_cast(&*(SourceLine.begin())) + (*I);
+  unsigned CharSize = llvm::getNumBytesForUTF8(*Begin);





Comment at: clang/lib/Frontend/TextDiagnostic.cpp:120
-  begin = reinterpret_cast(&*(SourceLine.begin() + *i));
-  end = begin + (SourceLine.size() - *i);
-

tbaeder wrote:
> I don't know what this computation of `end` means, but from the debug output 
> I added, it meant that a call to this function always converted the entire 
> line. I think.
It looks to me like an overly complicated way of asking for 
`SourceLine.bytes_end()`.



Comment at: clang/lib/Frontend/TextDiagnostic.cpp:121
+  unsigned CharSize = llvm::getNumBytesForUTF8(*Begin);
+  const unsigned char *End = Begin + CharSize;
 

This could assign `End` to a location past the end of the source line when the 
source line ends with an ill-formed code unit sequence (e.g., a truncated 
4-byte sequence). Constructing such a pointer is likely to get one in trouble.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150843

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

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

One last minor request and then I'm happy with this.




Comment at: clang/include/clang/Sema/ParsedAttr.h:705-707
+  // The move assignment operator is defined as deleted pending further
+  // motivation.
+  AttributePool &operator=(AttributePool &&pool) = delete;

Let's explicitly delete the copy assignment operator too since the copy 
constructor is deleted.


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

https://reviews.llvm.org/D150411

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

[PATCH] D150744: [NFC][CLANG] Fix uninitialized scalar field issues found by Coverity

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/include/clang/Parse/Parser.h:1190
   class ParseScopeFlags {
 Scope *CurScope;
+unsigned OldFlags = 0;

shafik wrote:
> @tahonermann I feel like we should have a default member initializer for any 
> member that by default is not initialized. I realize in this case there 
> currently should be no way for this to be bot initialized but that may not 
> stay true and I don't believe there will be a penalty for doing this since it 
> is initialized in the `mem-initializer-list` and therefore the default init 
> should be omitted when calling the constructor. 
I think that is very reasonable and I'm not opposed to such a policy. Per my 
other comment, the trade off to always initializing is that a default 
initializer can prevent use of tools like ubsan to discover when an appropriate 
(presumably non-default) value has not been assigned. I suspect (but have no 
data to draw on) that adding a default member initializer prevents more bugs 
than would be found by tools like ubsan detecting use of an uninitialized data 
member that should have a (non-default) assigned value.

I'm definitely in favor of using default member initializers over 
`mem-initializer-list`!


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

https://reviews.llvm.org/D150744

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

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:1790
 SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default;
+SemaDiagnosticBuilder &operator=(const SemaDiagnosticBuilder &) = delete;
 ~SemaDiagnosticBuilder();

aaronpuchert wrote:
> tahonermann wrote:
> > 
> Being explicit is certainly a good thing, but the C++ committee wants to go 
> into the direction of "copy/move operations are implicitly deleted if any of 
> them or the destructor is user-declared." See 
> [depr.impldec](https://eel.is/c++draft/depr.impldec). It is already partially 
> the case, for example here. So why do need to spell something out explicitly 
> when the language goes into the direction of being even more implicit?
I have low expectations of that deprecation ever being acted on. In fact, there 
have been discussions about un-deprecating it because of the lack of such 
intent.

The motivation for deprecation is not based on a judgement of whether implicit 
or explicit is better, but rather based on a safety argument; if a copy 
constructor or destructor is user-declared, that is probably because some form 
of resource management is needed that won't be performed by the defaulted copy 
assignment operator. Thus, defining the implicit copy assignment operator as 
deleted would avoid the possibility of the compiler injecting bugs in the 
program.

The rules for when the copy vs assignment operators are implicitly defined as 
deleted can be hard to remember. I think being explicit is useful if only to 
indicate that the author thought about whether such operations should be 
provided. Since we don't have a principled reason for defining these operations 
as deleted, it might not be a bad idea to add a comment that states something 
like "The copy/move assignment operator is defined as deleted pending further 
motivation".


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

https://reviews.llvm.org/D150411

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

[PATCH] D150744: [NFC][CLANG] Fix uninitialized scalar field issues found by Coverity

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

I'm not opposed to these changes, but please note that these changes mean is 
will no longer be possible to use ubsan to discover when these data members are 
used before having been assigned an appropriate value. That is only a concern 
when an appropriate value would differ from the member initializers added via 
this change. I haven't reviewed the changes in depth, so I don't know if there 
are any such cases for which such a concern is applicable.


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

https://reviews.llvm.org/D150744

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

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

[Tom Honermann via Phabricator via cfe-commits]

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

I added a few comments and suggested edits, but this is mostly looking good.




Comment at: clang/include/clang/Sema/Sema.h:1790
 SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default;
+SemaDiagnosticBuilder &operator=(const SemaDiagnosticBuilder &) = delete;
 ~SemaDiagnosticBuilder();





Comment at: clang/include/clang/Sema/Sema.h:1789-1791
+SemaDiagnosticBuilder &operator=(SemaDiagnosticBuilder &&D) = delete;
 SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default;
+SemaDiagnosticBuilder &operator=(const SemaDiagnosticBuilder &) = delete;

aaronpuchert wrote:
> Manna wrote:
> > @tahonermann This is follow-up comments from 
> > https://reviews.llvm.org/D149718?id=519331#inline-1452044. 
> > 
> > >>This change still declares a move assignment operator, but doesn't 
> > >>provide a definition. The move constructor is implemented in 
> > >>clang/lib/Sema/Sema.cpp, so I would expect to see the move assignment 
> > >>operator definition provided there as well.
> > 
> > I tried to define move assignment operator in ` clang/lib/Sema/Sema.cpp` 
> > but it failed because class Sema has deleted implicit copy assignment 
> > operator.
> > 
> > ```
> > /// Sema - This implements semantic analysis and AST building for C.
> > class Sema final {
> >   Sema(const Sema &) = delete;
> >   void operator=(const Sema &) = delete;
> > ```
> > It seems like support for assignment is not desired, We probably need 
> > deleted copy/move assignment operator.
> > 
> These are also implicitly deleted. Some code styles want this explicitly 
> spelled out, but I don't think ours does.
> I tried to define move assignment operator in  clang/lib/Sema/Sema.cpp but it 
> failed because class Sema has deleted implicit copy assignment operator.

It is still permissible to define a move assignment operator if the implicit 
copy assignment operator is deleted. See https://godbolt.org/z/sGaWd9M44.

I think it is fine to disable support for assignment for this class pending use 
cases. But, since a move constructor is explicitly defined, we should also be 
explicit above move assignment. I added a suggested edit. Without that change, 
I think Coverity will continue to complain.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:54
   BugReporterVisitor(BugReporterVisitor &&) {}
+  BugReporterVisitor &operator=(const BugReporterVisitor &) = delete;
   virtual ~BugReporterVisitor();

Here too; since a user-defined move constructor is declared, let's be explicit 
about move assignment.



Comment at: clang/lib/CodeGen/CGDebugInfo.h:833-837
+  ApplyDebugLocation &operator=(ApplyDebugLocation &&Other) {
+CGF = Other.CGF;
+Other.CGF = nullptr;
+return *this;
+  }

Good catch! Since the destructor uses `CGF`, the defaulted move assignment 
operator was wrong!



Comment at: clang/lib/CodeGen/EHScopeStack.h:151
 Cleanup(Cleanup &&) {}
+Cleanup &operator=(const Cleanup &) = delete;
 Cleanup() = default;

Here too; since a user-declared move constructor is present, let's be explicit 
about support for move assignment.


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

https://reviews.llvm.org/D150411

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

[PATCH] D150291: [Clang] Rename internal type identifier(s) for __bf16 to BF16Ty

[Tom Honermann via Phabricator via cfe-commits]

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

> I do wonder if we need two bfloat implementations, but for that I'll leave a 
> comment on D149573 .

Given the discussions occurring in D149573 , 
let's hold off on landing this for now. It is sounding like we might have 
direction for repurposing `__bf16` for `std::bfloat16_t` (and a future 
`_BFloat16` C type; with `__bf16` retained as an alternate spelling). If we go 
in that direction, then we would presumably want a change that goes in the 
opposite direction of this patch; a change that migrates "bf16" names towards 
"bfloat16". Let's focus on confirming that direction first. I'll mark this as 
requesting changes for now while we figure this out.


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

https://reviews.llvm.org/D150291

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

[PATCH] D150291: [Clang] Rename internal type identifier(s) for __bf16 to BF16Ty

[Tom Honermann via Phabricator via cfe-commits]

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

Thanks for all the updates @codemzs! I'm going to go ahead and accept. But 
please wait a few days for recently subscribed folks to have a chance to 
comment before landing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150291

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