[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Tom,
Thanks for the fixes! The patch looks good to me now. I have only a small nit 
inline.




Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());

aaron.ballman wrote:
> aaron.ballman wrote:
> > a_sidorin wrote:
> > > aaron.ballman wrote:
> > > > Please don't use `auto` here; the type isn't spelled out in the 
> > > > initialization.
> > > Hi Aaron,
> > > importSeq() is a helper targeted to simplify a lot of imports done during 
> > > AST node import and related error handling. The type of this 
> > > `importSeq()` expression is `Expected > > SourceLocation, SourceLocation, QualType>>`, so I think it's better to 
> > > leave it as `auto`.
> > Wow. I really dislike that we basically *have* to hide the type information 
> > because it's just too ugly to spell. I understand why we're using `auto` 
> > based on your explanation, but it basically means I can't understand what 
> > this code is doing without a lot more effort.
> > 
> > Let's keep the `auto`, but let's please stop using type compositions that 
> > make it considerably harder to review the code. This feel like a misuse of 
> > `std::tuple`.
> After staring at this for longer, I no longer think it's a misuse of 
> `std::tuple`, just... an unfortunate side-effect of the design of 
> `importSeq()`. I don't have a good idea for how to make this easier on 
> reviewers and other people who aren't frequently looking at the AST importing 
> code. :-(
I think it is a case where the type is not even important. With C++17, we would 
just write:
```auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), ...)
if (Imp)
  return Imp.takeError();
auto [ToCond, ToLHS, ToRHS] = *Imp;
```
The exact type is not really needed here. However, if you have any ideas on how 
to improve this and make the type more explicit - they are always welcome.



Comment at: unittests/AST/ASTImporterTest.cpp:581
+  // Don't try to match the template contents if template parsing is delayed.
+  if (std::find(std::begin(Args), std::end(Args),
+"-fdelayed-template-parsing") != Args.end()) {

LLVm has a set of nice helpers for working with containers in range -like 
style. I'd recommend you to use llvm::find here:
`if (llvm::find(Args), "-fdelayed-template-parsing") != Args.end()) { `


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



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

[PATCH] D58573: [analyzer] Move UninitializedObject out of alpha

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

Szelethus created this revision.
Szelethus added reviewers: dcoughlin, NoQ, xazax.hun, rnkovacs, whisperity, 
a.sidorin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, szepet, baloghadamsoftware.

I've tested the checker on 5 open source projects with 3 different 
configurations:

http://cc.inf.elte.hu:15420/Default/#

I think there is room for discussion whether which package should this checker 
reside in, but in terms of development, I would argue that this is ready to go.

Shall I make some other tests?


Repository:
  rC Clang

https://reviews.llvm.org/D58573

Files:
  docs/analyzer/checkers.rst
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object-no-dereference.cpp
  test/Analysis/cxx-uninitialized-object-notes-as-warnings.cpp
  test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
  test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
  test/Analysis/cxx-uninitialized-object-unionlike-constructs.cpp
  test/Analysis/cxx-uninitialized-object.cpp
  test/Analysis/objcpp-uninitialized-object.mm
  www/analyzer/alpha_checks.html
  www/analyzer/available_checks.html

Index: www/analyzer/available_checks.html
===
--- www/analyzer/available_checks.html
+++ www/analyzer/available_checks.html
@@ -435,6 +435,121 @@
 } // warn
 
 
+
+
+cplusplus.UninitializedObject
+(C++)
+This checker reports uninitialized fields in objects created after a constructor
+call. It doesn't only find direct uninitialized fields, but rather makes a deep
+inspection of the object, analyzing all of it's fields subfields. 
+The checker regards inherited fields as direct fields, so one will recieve
+warnings for uninitialized inherited data members as well. 
+
+It has several options:
+
+  
+"Pedantic" (boolean). If its not set or is set to false, the
+checker won't emit warnings for objects that don't have at least one
+initialized field. This may be set with 
+-analyzer-config cplusplus.UninitializedObject:Pedantic=true.
+  
+  
+"NotesAsWarnings" (boolean). If set to true, the checker will
+emit a warning for each uninitalized field, as opposed to emitting one
+warning per constructor call, and listing the uninitialized fields that
+belongs to it in notes. Defaults to false. 
+-analyzer-config cplusplus.UninitializedObject:NotesAsWarnings=true.
+  
+  
+"CheckPointeeInitialization" (boolean). If set to false, the
+checker will not analyze the pointee of pointer/reference fields, and will
+only check whether the object itself is initialized. Defaults to false. 
+-analyzer-config cplusplus.UninitializedObject:CheckPointeeInitialization=true.
+  
+  
+"IgnoreRecordsWithField" (string). If supplied, the checker
+will not analyze structures that have a field with a name or type name that
+matches the given pattern. Defaults to "".
+
+-analyzer-config cplusplus.UninitializedObject:IgnoreRecordsWithField="[Tt]ag|[Kk]ind".
+  
+
+
+
+// With Pedantic and CheckPointeeInitialization set to true
+
+struct A {
+  struct B {
+int x; // note: uninitialized field 'this->b.x'
+   // note: uninitialized field 'this->bptr->x'
+int y; // note: uninitialized field 'this->b.y'
+   // note: uninitialized field 'this->bptr->y'
+  };
+  int *iptr; // note: uninitialized pointer 'this->iptr'
+  B b;
+  B *bptr;
+  char *cptr; // note: uninitialized pointee 'this->cptr'
+
+  A (B *bptr, char *cptr) : bptr(bptr), cptr(cptr) {}
+};
+
+void f() {
+  A::B b;
+  char c;
+  A a(&b, &c); // warning: 6 uninitialized fields
+   //  after the constructor call
+}
+
+
+// With Pedantic set to false and
+// CheckPointeeInitialization set to true
+// (every field is uninitialized)
+
+struct A {
+  struct B {
+int x;
+int y;
+  };
+  int *iptr;
+  B b;
+  B *bptr;
+  char *cptr;
+
+  A (B *bptr, char *cptr) : bptr(bptr), cptr(cptr) {}
+};
+
+void f() {
+  A::B b;
+  char c;
+  A a(&b, &c); // no warning
+}
+
+
+// With Pedantic and CheckPointeeInitialization set to false
+// (pointees are regarded as initialized)
+
+struct A {
+  struct B {
+int x; // note: uninitialized field 'this->b.x'
+int y; // note: uninitialized field 'this->b.y'
+  };
+  int *iptr; // note: uninitialized pointer 'this->iptr'
+  B b;
+  B *bptr;
+  char *cptr;
+
+  A (B *bptr, char *cptr) : bptr(bptr), cptr(cptr) {}
+};
+
+void f() {
+  A::B b;
+  char c;
+  A a(&b, &c); // warning: 3 uninitialized fields
+   //  after the constructor call
+}
+
+
+
 
 
 
Index: www/analyzer/alpha_checks.html
===
--- www/analyzer/alpha_checks.html
+++ www/analyzer/alpha_checks.html
@@ -444,120 +444,6 @@
 

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

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

Szelethus accepted this revision.
Szelethus added a comment.

But, as a work-in-progress alpha checker, the direction is set and looks great. 
Please let @NoQ have the final say.


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

https://reviews.llvm.org/D50488



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

[PATCH] D57660: [Sema] SequenceChecker: Handle references, members and structured bindings.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 188043.
riccibruno retitled this revision from "[Sema] SequenceChecker: Handle 
references and members" to "[Sema] SequenceChecker: Handle references, members 
and structured bindings.".
riccibruno edited the summary of this revision.

Repository:
  rC Clang

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

https://reviews.llvm.org/D57660

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaChecking.cpp
  test/SemaCXX/warn-unsequenced.cpp

Index: test/SemaCXX/warn-unsequenced.cpp
===
--- test/SemaCXX/warn-unsequenced.cpp
+++ test/SemaCXX/warn-unsequenced.cpp
@@ -160,17 +160,21 @@
   void member_f(S1 &s);
 };
 
+class SomeClass { public: static int x; };
+union SomeUnion { public: static int x; };
+
 void S1::member_f(S1 &s) {
-  ++a + ++a; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
-  a + ++a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  ++a + ++a; // cxx11-warning {{multiple unsequenced modifications to member 'a'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'a'}}
+  a + ++a; // cxx11-warning {{unsequenced modification and access to member 'a'}}
+   // cxx17-warning@-1 {{unsequenced modification and access to member 'a'}}
   ++a + ++b; // no-warning
   a + ++b; // no-warning
 
-  // TODO: Warn here.
-  ++s.a + ++s.a; // no-warning TODO {{multiple unsequenced modifications to}}
-  s.a + ++s.a; // no-warning TODO {{unsequenced modification and access to}}
+  ++s.a + ++s.a; // cxx11-warning {{multiple unsequenced modifications to member 'a' of 's'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'a' of 's'}}
+  s.a + ++s.a; // cxx11-warning {{unsequenced modification and access to member 'a' of 's'}}
+   // cxx17-warning@-1 {{unsequenced modification and access to member 'a' of 's'}}
   ++s.a + ++s.b; // no-warning
   s.a + ++s.b; // no-warning
 
@@ -180,16 +184,18 @@
   a + ++s.b; // no-warning
 
   // TODO Warn here for bit-fields in the same memory location.
-  ++bf1 + ++bf1; // cxx11-warning {{multiple unsequenced modifications to 'bf1'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'bf1'}}
-  bf1 + ++bf1; // cxx11-warning {{unsequenced modification and access to 'bf1'}}
-   // cxx17-warning@-1 {{unsequenced modification and access to 'bf1'}}
+  ++bf1 + ++bf1; // cxx11-warning {{multiple unsequenced modifications to member 'bf1'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'bf1'}}
+  bf1 + ++bf1; // cxx11-warning {{unsequenced modification and access to member 'bf1'}}
+   // cxx17-warning@-1 {{unsequenced modification and access to member 'bf1'}}
   ++bf1 + ++bf2; // no-warning TODO {{multiple unsequenced modifications to}}
   bf1 + ++bf2; // no-warning TODO {{unsequenced modification and access to}}
 
   // TODO Warn here for bit-fields in the same memory location.
-  ++s.bf1 + ++s.bf1; // no-warning TODO {{multiple unsequenced modifications to}}
-  s.bf1 + ++s.bf1; // no-warning TODO {{unsequenced modification and access to}}
+  ++s.bf1 + ++s.bf1; // cxx11-warning {{multiple unsequenced modifications to member 'bf1' of 's'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to member 'bf1' of 's'}}
+  s.bf1 + ++s.bf1; // cxx11-warning {{unsequenced modification and access to member 'bf1' of 's'}}
+   // cxx17-warning@-1 {{unsequenced modification and access to member 'bf1' of 's'}}
   ++s.bf1 + ++s.bf2; // no-warning TODO {{multiple unsequenced modifications to}}
   s.bf1 + ++s.bf2; // no-warning TODO {{unsequenced modification and access to}}
 
@@ -203,19 +209,29 @@
   Der &d_ref = d;
   S1 &s1_ref = d_ref;
 
-  ++s1_ref.a + ++d_ref.a; // no-warning TODO {{multiple unsequenced modifications to member 'a' of 'd'}}
-  ++s1_ref.a + d_ref.a; // no-warning TODO {{unsequenced modification and access to member 'a' of 'd'}}
+  ++s1_ref.a + ++d_ref.a; // cxx11-warning {{multiple unsequenced modifications to member 'a' of 'd'}}
+  // cxx17-warning@-1 {{multiple unsequenced modifications to member 'a' of 'd'}}
+  ++s1_ref.a + d_ref.a; // cxx11-warning {{unsequenced modification and access to member 'a' of 'd'}}
+// cxx17-warning@-1 {{unsequenced modification and access to member 'a' of 'd'}}
   ++s1_ref.a + ++d_ref.b; // no-warning
   ++s1_ref.a + d_ref.b; // no-warning
 
-  ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
- // cxx17-warning@-1 {{multiple unsequenced modifications to 'x'}}
-  ++x + x; // cxx11-warning {{unsequenced mo

[PATCH] D57747: [Sema] SequenceChecker: Fix handling of operator ||, && and ?:

[Bruno Ricci via Phabricator via cfe-commits]

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

Rebased on D57660 . No need to look at it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57747

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/warn-unsequenced.c
  test/SemaCXX/warn-unsequenced.cpp

Index: test/SemaCXX/warn-unsequenced.cpp
===
--- test/SemaCXX/warn-unsequenced.cpp
+++ test/SemaCXX/warn-unsequenced.cpp
@@ -4,6 +4,8 @@
 // RUN:-Wunsequenced -Wno-c++17-extensions -Wno-c++14-extensions %s
 
 int f(int, int = 0);
+int g1();
+int g2(int);
 
 struct A {
   int x, y;
@@ -79,24 +81,28 @@
   A { ++a, a++ }.x + A { ++a, a++ }.y; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
 
-  (xs[2] && (a = 0)) + a; // ok
+  (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 && (a = 0)) + a; // ok
   (1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
 
-  (xs[3] || (a = 0)) + a; // ok
+  (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (1 || (a = 0)) + a; // ok
 
-  (xs[4] ? a : ++a) + a; // ok
+  (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (0 ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (1 ? a : ++a) + a; // ok
   (0 ? a : a++) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (1 ? a : a++) + a; // ok
-  (xs[5] ? ++a : ++a) + a; // FIXME: warn here
+  (xs[5] ? ++a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
 
   (++a, xs[6] ? ++a : 0) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
   // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
@@ -122,10 +128,13 @@
   // unconditional.
   a = a++ && f(a, a);
 
-  // This has undefined behavior if a != 0. FIXME: We should diagnose this.
-  (a && a++) + a;
+  // This has undefined behavior if a != 0.
+  (a && a++) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
 
-  (xs[7] && ++a) * (!xs[7] && ++a); // ok
+  // FIXME: Don't warn here.
+  (xs[7] && ++a) * (!xs[7] && ++a); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
 
   xs[0] = (a = 1, a); // ok
   (a -= 128) &= 128; // ok
@@ -135,13 +144,64 @@
  // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   xs[8] ? 0 : ++a + a++; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
  // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
-  xs[8] ? ++a : a++; // ok
+  xs[8] ? ++a : a++; // no-warning
+  xs[8] ? a+=1 : a+= 2; // no-warning
+  (xs[8] ? a+=1 : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+  // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  (xs[8] ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+  // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  (xs[8] ? a : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+   // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+  a = (xs[8] ? a+=1 : a+= 2); // no-warning
+  a += (xs[8] ? a+=1 : a+= 2); // cxx11-warning {{unsequenced modification and access to 'a'}}
+   // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+
+  (false ? a+=1 : a) = a; // no-warning
+  (true ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // TODO cxx17-warning@-1 {{u

[PATCH] D58137: [clang-tidy] Add the abseil-time-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:97
+void TimeSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *BinOp = Result.Nodes.getNodeAs("binop");
+  std::string inverse_name =

hwright wrote:
> JonasToth wrote:
> > Could you please split this function up into smaller ones. There are three 
> > or four distinct cases that are easier to comprehend in isolation.
> The actual bodies of these if-statements are only one or two separate 
> statements themselves.  Moving those to separate functions seems like it 
> would just obfuscate things a bit.
IMHO they are complicated statements and hide what is being done. Wrapping them 
in a function with a name that states what is done seems appropriate.



Comment at: clang-tidy/abseil/TimeSubtractionCheck.h:19
+/// Finds and fixes `absl::Time` subtraction expressions to do subtraction
+/// in the Time domain instead of the numeric domain.
+///

hwright wrote:
> JonasToth wrote:
> > nit: 'Time' domain
> This doesn't refer to a type, but a library system, so it probably isn't 
> appropriate to quote it.
> 
> (Just has how one wouldn't quote "frequency" when talking about "the 
> frequency domain" of a Fourier transform.)
ah true, but then time would be written small i guess.



Comment at: test/clang-tidy/abseil-time-subtraction.cpp:12
+
+  d = absl::Hours(absl::ToUnixHours(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time 
domain [abseil-time-subtraction]

hwright wrote:
> JonasToth wrote:
> > please add tests where `x` itself is a calculation with different 
> > precedence of its operators (multiplication, addition) to ensure these 
> > cases are transformed properly as well.
> This doesn't actually matter in this case: `x` will be wrapped in a function 
> call.
> 
> It does matter in the case where we //unwrap// the first argument (below) and 
> I've already got a test which uses multiplication in this case.  I've also 
> added one for division.
Yes, it should not matter if `x` is an expr itself or just a variable. Thats 
why it should be tested its actually true.


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

https://reviews.llvm.org/D58137



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

[PATCH] D58297: [Sema] SequenceChecker: C++17 sequencing rules for built-in operators <<, >>, .*, ->*, =, op=

[Bruno Ricci via Phabricator via cfe-commits]

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

Rebased on D57660 . No need to look at it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58297

Files:
  lib/Sema/SemaChecking.cpp
  test/CXX/drs/dr2xx.cpp
  test/CXX/drs/dr6xx.cpp
  test/SemaCXX/warn-unsequenced.cpp

Index: test/SemaCXX/warn-unsequenced.cpp
===
--- test/SemaCXX/warn-unsequenced.cpp
+++ test/SemaCXX/warn-unsequenced.cpp
@@ -26,7 +26,6 @@
   a + a++; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   a = a++; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-   // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   ++ ++a; // ok
   (a++, a++); // ok
   ++a + ++a; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
@@ -36,13 +35,10 @@
   (a++, a) = 0; // ok, increment is sequenced before value computation of LHS
   a = xs[++a]; // ok
   a = xs[a++]; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-   // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   (a ? xs[0] : xs[1]) = ++a; // cxx11-warning {{unsequenced modification and access to 'a'}}
- // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   a = (++a, ++a); // ok
   a = (a++, ++a); // ok
   a = (a++, a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-  // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   f(a, a); // ok
   f(a = 0, a); // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
@@ -61,7 +57,6 @@
   (++a, a) += 1; // ok
   a = ++a; // ok
   a += ++a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-// TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
 
   A agg1 = { a++, a++ }; // ok
   A agg2 = { a++ + a, a++ }; // cxx11-warning {{unsequenced modification and access to 'a'}}
@@ -77,7 +72,6 @@
   a = S { ++a, a++ }.n; // ok
   A { ++a, a++ }.x; // ok
   a = A { ++a, a++ }.x; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-// TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   A { ++a, a++ }.x + A { ++a, a++ }.y; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
 
@@ -112,14 +106,10 @@
   a += (a++, a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
  // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
 
-  int *p = xs;
-  a = *(a++, p); // ok
   a = a++ && a; // ok
-  p[(long long unsigned)(p = 0)]; // cxx11-warning {{unsequenced modification and access to 'p'}}
 
   A *q = &agg1;
   (q = &agg2)->y = q->x; // cxx11-warning {{unsequenced modification and access to 'q'}}
- // TODO cxx17-warning@-1 {{unsequenced modification and access to 'q'}}
 
   // This has undefined behavior if a == 0; otherwise, the side-effect of the
   // increment is sequenced before the value computation of 'f(a, a)', which is
@@ -147,20 +137,14 @@
   xs[8] ? ++a : a++; // no-warning
   xs[8] ? a+=1 : a+= 2; // no-warning
   (xs[8] ? a+=1 : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-  // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (xs[8] ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-  // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (xs[8] ? a : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-   // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   a = (xs[8] ? a+=1 : a+= 2); // no-warning
   a += (xs[8] ? a+=1 : a+= 2); // cxx11-warning {{unsequenced modification and access to 'a'}}
-   // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
 
   (false ? a+=1 : a) = a; // no-warning
   (true ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
- // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (false ? a : a+=2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-  // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (true ? a : a+=2) = a; // no-warning
 
   xs[8] && (++a + a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
@@ -180,19 +164,15 @@
   a = ((a++, false) || (a++, false

[PATCH] D45978: dllexport const variables must have external linkage.

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

Let's see if I have included every thing mentioned. Thanks.


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

https://reviews.llvm.org/D45978



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

[PATCH] D45978: dllexport const variables must have external linkage.

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 188047.
zahiraam marked 2 inline comments as done.
Herald added subscribers: jdoerfert, jfb, mgrang, srhines.

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

https://reviews.llvm.org/D45978

Files:
  lib/Sema/SemaDecl.cpp
  mypatch.patch
  test/CodeGen/dllexport-1.c
  test/Sema/dllexport-1.cpp
  test/Sema/dllexport-2.cpp



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

[PATCH] D57659: [Sema] SequenceChecker: Add some comments + related small NFCs in preparation of the following patches

[Bruno Ricci via Phabricator via cfe-commits]

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

Rebased.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57659

Files:
  lib/Sema/SemaChecking.cpp

Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -11736,20 +11736,38 @@
 UK_Count = UK_ModAsSideEffect + 1
   };
 
+  /// Bundle together a sequencing region and the expression corresponding
+  /// to a specific usage. One Usage is stored for each usage kind in UsageInfo.
   struct Usage {
-Expr *Use;
+Expr *UsageExpr = nullptr;
 SequenceTree::Seq Seq;
-
-Usage() : Use(nullptr), Seq() {}
+Usage() = default;
+Usage(Expr *UsageExpr, SequenceTree::Seq Seq)
+: UsageExpr(UsageExpr), Seq(Seq) {}
   };
 
-  struct UsageInfo {
-Usage Uses[UK_Count];
+  class UsageInfo {
+/// The expressions corresponding to each usage kind.
+Expr *UsageExprs[UK_Count]{};
+
+/// The sequencing regions corresponding to each usage kind.
+SequenceTree::Seq Seqs[UK_Count]{};
 
-/// Have we issued a diagnostic for this variable already?
-bool Diagnosed;
+/// Have we issued a diagnostic for this object already?
+bool Diagnosed = false;
 
-UsageInfo() : Uses(), Diagnosed(false) {}
+  public:
+Usage getUsage(UsageKind UK) const {
+  assert(UK < UK_Count && "Invalid UsageKind!");
+  return Usage{UsageExprs[UK], Seqs[UK]};
+}
+void setUsage(UsageKind UK, Usage U) {
+  assert(UK < UK_Count && "Invalid UsageKind!");
+  UsageExprs[UK] = U.UsageExpr;
+  Seqs[UK] = U.Seq;
+}
+void markDiagnosed() { Diagnosed = true; }
+bool alreadyDiagnosed() const { return Diagnosed; }
   };
   using UsageInfoMap = llvm::SmallDenseMap;
 
@@ -11784,11 +11802,14 @@
 }
 
 ~SequencedSubexpression() {
-  for (auto &M : llvm::reverse(ModAsSideEffect)) {
-UsageInfo &U = Self.UsageMap[M.first];
-auto &SideEffectUsage = U.Uses[UK_ModAsSideEffect];
-Self.addUsage(U, M.first, SideEffectUsage.Use, UK_ModAsValue);
-SideEffectUsage = M.second;
+  for (std::pair &M : llvm::reverse(ModAsSideEffect)) {
+// Add a new usage with usage kind UK_ModAsValue, and then restore
+// the previous usage with UK_ModAsSideEffect (thus clearing it if
+// the previous one was empty).
+UsageInfo &UI = Self.UsageMap[M.first];
+Usage SideEffectUsage = UI.getUsage(UK_ModAsSideEffect);
+Self.addUsage(M.first, UI, SideEffectUsage.UsageExpr, UK_ModAsValue);
+UI.setUsage(UK_ModAsSideEffect, M.second);
   }
   Self.ModAsSideEffect = OldModAsSideEffect;
 }
@@ -11851,28 +11872,34 @@
   }
 
   /// Note that an object was modified or used by an expression.
-  void addUsage(UsageInfo &UI, Object O, Expr *Ref, UsageKind UK) {
-Usage &U = UI.Uses[UK];
-if (!U.Use || !Tree.isUnsequenced(Region, U.Seq)) {
+  /// UI is the UsageInfo for the object O as obtained via the UsageMap.
+  void addUsage(Object O, UsageInfo &UI, Expr *UsageExpr, UsageKind UK) {
+// Get the old usage for the given object and usage kind.
+Usage U = UI.getUsage(UK);
+if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq)) {
+  // If we have a modification as side effect and are in a sequenced
+  // subexpression, save the old Usage so that we can restore it later
+  // in SequencedSubexpression::~SequencedSubexpression.
   if (UK == UK_ModAsSideEffect && ModAsSideEffect)
 ModAsSideEffect->push_back(std::make_pair(O, U));
-  U.Use = Ref;
-  U.Seq = Region;
+  // Then record the new usage with the current sequencing region.
+  UI.setUsage(UK, Usage(UsageExpr, Region));
 }
   }
 
   /// Check whether a modification or use conflicts with a prior usage.
-  void checkUsage(Object O, UsageInfo &UI, Expr *Ref, UsageKind OtherKind,
+  /// UI is the UsageInfo for the object O as obtained via the UsageMap.
+  void checkUsage(Object O, UsageInfo &UI, Expr *UsageExpr, UsageKind OtherKind,
   bool IsModMod) {
-if (UI.Diagnosed)
+if (UI.alreadyDiagnosed())
   return;
 
-const Usage &U = UI.Uses[OtherKind];
-if (!U.Use || !Tree.isUnsequenced(Region, U.Seq))
+Usage U = UI.getUsage(OtherKind);
+if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq))
   return;
 
-Expr *Mod = U.Use;
-Expr *ModOrUse = Ref;
+Expr *Mod = U.UsageExpr;
+Expr *ModOrUse = UsageExpr;
 if (OtherKind == UK_Use)
   std::swap(Mod, ModOrUse);
 
@@ -11880,32 +11907,60 @@
  IsModMod ? diag::warn_unsequenced_mod_mod
   : diag::warn_unsequenced_mod_use)
   << O << SourceRange(ModOrUse->getExprLoc());
-UI.Diagnosed = true;
+UI.markDiagnosed();
   }
 
-  void notePreUse(Object O, Expr *Use) {
-UsageInfo &U = UsageMap[O]

[PATCH] D58579: [Sema] SequenceChecker: C++17 sequencing rule for call expression.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added reviewers: rsmith, aaron.ballman, Rakete.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.

In C++17 the postfix-expression of a call expression is sequenced before each 
expression in the expression-list and any default argument.


Repository:
  rC Clang

https://reviews.llvm.org/D58579

Files:
  include/clang/AST/Expr.h
  lib/Sema/SemaChecking.cpp
  test/SemaCXX/warn-unsequenced.cpp

Index: test/SemaCXX/warn-unsequenced.cpp
===
--- test/SemaCXX/warn-unsequenced.cpp
+++ test/SemaCXX/warn-unsequenced.cpp
@@ -15,7 +15,6 @@
   int n;
 };
 
-// TODO: Implement the C++17 sequencing rules.
 void test() {
   int a;
   int xs[10];
@@ -256,6 +255,17 @@
   p[i++] = (i = 42); // cxx11-warning {{multiple unsequenced modifications to 'i'}}
   p++[i++] = (i = p ? i++ : i++); // cxx11-warning {{unsequenced modification and access to 'p'}}
   // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+
+  (i++, f)(i++, 42); // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  (i++ + i++, f)(42, 42); // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  // cxx17-warning@-1 {{multiple unsequenced modifications to 'i'}}
+  int (*pf)(int, int);
+  (pf = f)(pf != nullptr, pf != nullptr); // cxx11-warning {{unsequenced modification and access to 'pf'}}
+  pf((pf = f) != nullptr, 42); // cxx11-warning {{unsequenced modification and access to 'pf'}}
+  f((pf = f, 42), (pf = f, 42)); // cxx11-warning {{multiple unsequenced modifications to 'pf'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to 'pf'}}
+  pf((pf = f) != nullptr, pf == nullptr); // cxx11-warning {{unsequenced modification and access to 'pf'}}
+  // cxx17-warning@-1 {{unsequenced modification and access to 'pf'}}
 }
 
 namespace members {
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -12490,6 +12490,11 @@
   }
 
   void VisitCallExpr(const CallExpr *CE) {
+// FIXME: CXXNewExpr and CXXDeleteExpr implicitly call functions.
+
+if (CE->isUnevaluatedBuiltinCall(Context))
+  return;
+
 // C++11 [intro.execution]p15:
 //   When calling a function [...], every value computation and side effect
 //   associated with any argument expression, or with the postfix expression
@@ -12497,9 +12502,50 @@
 //   expression or statement in the body of the function [and thus before
 //   the value computation of its result].
 SequencedSubexpression Sequenced(*this);
-Base::VisitCallExpr(CE);
 
-// FIXME: CXXNewExpr and CXXDeleteExpr implicitly call functions.
+// C++17 [expr.call]p5
+//   The postfix-expression is sequenced before each expression in the
+//   expression-list and any default argument. [...]
+SequenceTree::Seq CalleeRegion;
+SequenceTree::Seq OtherRegion;
+if (SemaRef.getLangOpts().CPlusPlus17) {
+  CalleeRegion = Tree.allocate(Region);
+  OtherRegion = Tree.allocate(Region);
+} else {
+  CalleeRegion = Region;
+  OtherRegion = Region;
+}
+SequenceTree::Seq OldRegion = Region;
+
+llvm::ArrayRef SubExprs = CE->getRawSubExprs();
+const Stmt *const *SubExpr = SubExprs.begin();
+const Stmt *const *EndExpr = SubExprs.end();
+
+// Visit the callee expression first.
+Region = CalleeRegion;
+if (SemaRef.getLangOpts().CPlusPlus17) {
+  SequencedSubexpression Sequenced(*this);
+  Visit(cast(*SubExpr));
+} else {
+  Visit(cast(*SubExpr));
+}
+assert(cast(*SubExpr) == CE->getCallee() &&
+   "The first sub-expression of the call expression is"
+   " assumed to be the callee expression!");
+++SubExpr;
+
+// Then visit the argument expressions. We are not able to use
+// CallExpr::arguments since we also need to visit the pre-argument
+// expressions.
+Region = OtherRegion;
+for (; SubExpr != EndExpr; ++SubExpr)
+  Visit(cast(*SubExpr));
+
+Region = OldRegion;
+if (SemaRef.getLangOpts().CPlusPlus17) {
+  Tree.merge(CalleeRegion);
+  Tree.merge(OtherRegion);
+}
   }
 
   void VisitCXXConstructExpr(const CXXConstructExpr *CCE) {
Index: include/clang/AST/Expr.h
===
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -2639,7 +2639,7 @@
   /// a CallExpr without going through the slower virtual child_iterator
   /// interface.  This provides efficient reverse iteration of the
   /// subexpressions.  This is currently used for CFG construction.
-  ArrayRef getRawSubExprs() {
+  ArrayRef getRawSubExprs() const {
 return llvm::makeArrayRef(getTrailingStmts(),
 

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

> I'm not quite sure what this differential is about, but i feel like 
> mentioning ExprMutationAnalyzer lib in clang-tidy / clang-tools-extra.

Alternatively perhaps you could re-use `getMemoryLocation()` from D57660 
. It would handle for you members, references, 
structured bindings +more in a relatively self-contained code.


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

https://reviews.llvm.org/D58035



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

r354727 - [Sema][NFC] SequenceChecker: More tests in preparation for D57660

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Sat Feb 23 08:25:00 2019
New Revision: 354727

URL: http://llvm.org/viewvc/llvm-project?rev=354727&view=rev
Log:
[Sema][NFC] SequenceChecker: More tests in preparation for D57660


Modified:
cfe/trunk/test/SemaCXX/warn-unsequenced.cpp

Modified: cfe/trunk/test/SemaCXX/warn-unsequenced.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-unsequenced.cpp?rev=354727&r1=354726&r2=354727&view=diff
==
--- cfe/trunk/test/SemaCXX/warn-unsequenced.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-unsequenced.cpp Sat Feb 23 08:25:00 2019
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 -fsyntax-only -verify=cxx11 -std=c++11 -Wno-unused 
-Wno-uninitialized -Wunsequenced %s
-// RUN: %clang_cc1 -fsyntax-only -verify=cxx17 -std=c++17 -Wno-unused 
-Wno-uninitialized -Wunsequenced %s
+// RUN: %clang_cc1 -fsyntax-only -verify=cxx11 -std=c++11 -Wno-unused 
-Wno-uninitialized \
+// RUN:-Wunsequenced -Wno-c++17-extensions -Wno-c++14-extensions %s
+// RUN: %clang_cc1 -fsyntax-only -verify=cxx17 -std=c++17 -Wno-unused 
-Wno-uninitialized \
+// RUN:-Wunsequenced -Wno-c++17-extensions -Wno-c++14-extensions %s
 
 int f(int, int = 0);
 
@@ -154,13 +156,11 @@ struct S1 {
   unsigned bf2 : 2;
   unsigned a;
   unsigned b;
-
+  static unsigned x;
   void member_f(S1 &s);
 };
 
 void S1::member_f(S1 &s) {
-  int xs[10];
-
   ++a + ++a; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
  // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   a + ++a; // cxx11-warning {{unsequenced modification and access to 'a'}}
@@ -197,6 +197,25 @@ void S1::member_f(S1 &s) {
   bf1 + ++s.bf1; // no-warning
   ++bf1 + ++s.bf2; // no-warning
   bf1 + ++s.bf2; // no-warning
+
+  struct Der : S1 {};
+  Der d;
+  Der &d_ref = d;
+  S1 &s1_ref = d_ref;
+
+  ++s1_ref.a + ++d_ref.a; // no-warning TODO {{multiple unsequenced 
modifications to member 'a' of 'd'}}
+  ++s1_ref.a + d_ref.a; // no-warning TODO {{unsequenced modification and 
access to member 'a' of 'd'}}
+  ++s1_ref.a + ++d_ref.b; // no-warning
+  ++s1_ref.a + d_ref.b; // no-warning
+
+  ++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
+ // cxx17-warning@-1 {{multiple unsequenced modifications to 'x'}}
+  ++x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
+   // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
+  ++s.x + x; // no-warning TODO {{unsequenced modification and access to 
static member 'x' of 'S1'}}
+  ++this->x + x; // cxx11-warning {{unsequenced modification and access to 
'x'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 
'x'}}
+  ++d_ref.x + ++S1::x; // no-warning TODO {{unsequenced modification and 
access to static member 'x' of 'S1'}}
 }
 
 struct S2 {
@@ -319,6 +338,119 @@ void reference_f() {
 }
 } // namespace references
 
+namespace std {
+  using size_t = decltype(sizeof(0));
+  template struct tuple_size;
+  template struct tuple_element { using type = int; };
+}
+namespace bindings {
+
+  struct A { int x, y; };
+  typedef int B[2];
+  struct C { template int get(); };
+  struct D : A {};
+
+} // namespace bindings
+template<> struct std::tuple_size { enum { value = 2 }; };
+namespace bindings {
+void testa() {
+  A a;
+  {
+auto [x, y] = a;
+++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
+   // cxx17-warning@-1 {{multiple unsequenced modifications to 
'x'}}
+++x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
+++x + ++y; // no-warning
+++x + y; // no-warning
+++x + ++a.x; // no-warning
+++x + a.x; // no-warning
+  }
+  {
+auto &[x, y] = a;
+++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
+   // cxx17-warning@-1 {{multiple unsequenced modifications to 
'x'}}
+++x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
+++x + ++y; // no-warning
+++x + y; // no-warning
+++x + ++a.x; // no-warning TODO
+++x + a.x; // no-warning TODO
+  }
+}
+void testb() {
+  B b;
+  {
+auto [x, y] = b;
+++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
+   // cxx17-warning@-1 {{multiple unsequenced modifications to 
'x'}}
+++x + x; // cxx11-warning {{unsequenced modification and access to 'x'}}
+ // cxx17-warning@-1 {{unsequenced modification and access to 'x'}}
+++x + ++y; // no-warning
+++x + y; // no-warning
+++x + ++b[0]; // no-warning
+++x + b[0]; // no-warning
+  }
+  {
+auto &[x, y] = b;
+++x + ++x; // cxx11-warning {{multiple unsequenced modifications to 'x'}}
+   // cxx17-warning@-

r354728 - [NFC] Fix Wdocumentation warning in OMPToClause

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Sat Feb 23 08:40:30 2019
New Revision: 354728

URL: http://llvm.org/viewvc/llvm-project?rev=354728&view=rev
Log:
[NFC] Fix Wdocumentation warning in OMPToClause


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

Modified: cfe/trunk/include/clang/AST/OpenMPClause.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/OpenMPClause.h?rev=354728&r1=354727&r2=354728&view=diff
==
--- cfe/trunk/include/clang/AST/OpenMPClause.h (original)
+++ cfe/trunk/include/clang/AST/OpenMPClause.h Sat Feb 23 08:40:30 2019
@@ -4979,7 +4979,6 @@ class OMPToClause final : public OMPMapp
   /// \param MapperQualifierLoc C++ nested name specifier for the associated
   /// user-defined mapper.
   /// \param MapperIdInfo The identifier of associated user-defined mapper.
-  /// \param MapType Map type.
   /// \param Locs Locations needed to build a mappable clause. It includes 1)
   /// StartLoc: starting location of the clause (the clause keyword); 2)
   /// LParenLoc: location of '('; 3) EndLoc: ending location of the clause.


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

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());

a_sidorin wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > a_sidorin wrote:
> > > > aaron.ballman wrote:
> > > > > Please don't use `auto` here; the type isn't spelled out in the 
> > > > > initialization.
> > > > Hi Aaron,
> > > > importSeq() is a helper targeted to simplify a lot of imports done 
> > > > during AST node import and related error handling. The type of this 
> > > > `importSeq()` expression is `Expected > > > *, SourceLocation, SourceLocation, QualType>>`, so I think it's better 
> > > > to leave it as `auto`.
> > > Wow. I really dislike that we basically *have* to hide the type 
> > > information because it's just too ugly to spell. I understand why we're 
> > > using `auto` based on your explanation, but it basically means I can't 
> > > understand what this code is doing without a lot more effort.
> > > 
> > > Let's keep the `auto`, but let's please stop using type compositions that 
> > > make it considerably harder to review the code. This feel like a misuse 
> > > of `std::tuple`.
> > After staring at this for longer, I no longer think it's a misuse of 
> > `std::tuple`, just... an unfortunate side-effect of the design of 
> > `importSeq()`. I don't have a good idea for how to make this easier on 
> > reviewers and other people who aren't frequently looking at the AST 
> > importing code. :-(
> I think it is a case where the type is not even important. With C++17, we 
> would just write:
> ```auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), ...)
> if (Imp)
>   return Imp.takeError();
> auto [ToCond, ToLHS, ToRHS] = *Imp;
> ```
> The exact type is not really needed here. However, if you have any ideas on 
> how to improve this and make the type more explicit - they are always welcome.
I actually don't find the structured binding version to be any more readable 
than the wrapped-tuple version -- either way, I don't see the types. What makes 
this current code somewhat-okay to me is the `std::tie()` below; that at least 
lets me figure it out through reasoning. In the structured binding version, 
that would likely go away and I'd probably argue harder for this being an 
unfortunate design for maintainers and reviewers.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



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

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> jkorous wrote:
> > mgorny wrote:
> > > Why? I suppose this deserves a comment.
> > I'll add this comment:
> > 
> > // The file might have been removed just after we received the event.
> Wouldn't that cause removals to be reported twice?
Not quite sure if it can happen in practice but I'd suggest to accept this as 
potential occurrence and add it to documentation ("a 'removed' event may be 
reported twice). I think it is better to handle a definite "fact" (the file 
doesn't exist) than ignore it and assume the 'removed' event will eventually 
show up, or try to eliminate the double reporting which would over-complicate 
the implementation.

After all, if inotify() is not 100% reliable then there's already the 
possibility that you'll get a 'removed' event for a file that was not reported 
as 'added' before.


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

https://reviews.llvm.org/D58418



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

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

akyrtzi wrote:
> mgorny wrote:
> > jkorous wrote:
> > > mgorny wrote:
> > > > Why? I suppose this deserves a comment.
> > > I'll add this comment:
> > > 
> > > // The file might have been removed just after we received the event.
> > Wouldn't that cause removals to be reported twice?
> Not quite sure if it can happen in practice but I'd suggest to accept this as 
> potential occurrence and add it to documentation ("a 'removed' event may be 
> reported twice). I think it is better to handle a definite "fact" (the file 
> doesn't exist) than ignore it and assume the 'removed' event will eventually 
> show up, or try to eliminate the double reporting which would over-complicate 
> the implementation.
> 
> After all, if inotify() is not 100% reliable then there's already the 
> possibility that you'll get a 'removed' event for a file that was not 
> reported as 'added' before.
I see this as a partial workaround for race condition. You 'fix' it if removal 
happens between inotify reporting the event and you returning it. You don't if 
removal happens after you push it to events. Therefore, the caller still needs 
to account for 'added' event being obsolete. I don't really see a purpose in 
trying to workaround the problem partially here if the caller still needs to 
account for it. Effectively, you're replacing one normal case and one corner 
case with one normal case and two corner cases.


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

https://reviews.llvm.org/D58418



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

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > jkorous wrote:
> > > > mgorny wrote:
> > > > > Why? I suppose this deserves a comment.
> > > > I'll add this comment:
> > > > 
> > > > // The file might have been removed just after we received the event.
> > > Wouldn't that cause removals to be reported twice?
> > Not quite sure if it can happen in practice but I'd suggest to accept this 
> > as potential occurrence and add it to documentation ("a 'removed' event may 
> > be reported twice). I think it is better to handle a definite "fact" (the 
> > file doesn't exist) than ignore it and assume the 'removed' event will 
> > eventually show up, or try to eliminate the double reporting which would 
> > over-complicate the implementation.
> > 
> > After all, if inotify() is not 100% reliable then there's already the 
> > possibility that you'll get a 'removed' event for a file that was not 
> > reported as 'added' before.
> I see this as a partial workaround for race condition. You 'fix' it if 
> removal happens between inotify reporting the event and you returning it. You 
> don't if removal happens after you push it to events. Therefore, the caller 
> still needs to account for 'added' event being obsolete. I don't really see a 
> purpose in trying to workaround the problem partially here if the caller 
> still needs to account for it. Effectively, you're replacing one normal case 
> and one corner case with one normal case and two corner cases.
I was mainly pointing out that the client already has to be prepared for a 
'removed' event that does not have an associated 'added' event, regardless of 
what this code is doing. Therefore a potential double 'removed' event doesn't 
add complexity to the client.

Could you clarify how the code should handle the inability to get the mod time 
? Should it ignore the event ?


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

https://reviews.llvm.org/D58418



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

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

akyrtzi wrote:
> mgorny wrote:
> > akyrtzi wrote:
> > > mgorny wrote:
> > > > jkorous wrote:
> > > > > mgorny wrote:
> > > > > > Why? I suppose this deserves a comment.
> > > > > I'll add this comment:
> > > > > 
> > > > > // The file might have been removed just after we received the event.
> > > > Wouldn't that cause removals to be reported twice?
> > > Not quite sure if it can happen in practice but I'd suggest to accept 
> > > this as potential occurrence and add it to documentation ("a 'removed' 
> > > event may be reported twice). I think it is better to handle a definite 
> > > "fact" (the file doesn't exist) than ignore it and assume the 'removed' 
> > > event will eventually show up, or try to eliminate the double reporting 
> > > which would over-complicate the implementation.
> > > 
> > > After all, if inotify() is not 100% reliable then there's already the 
> > > possibility that you'll get a 'removed' event for a file that was not 
> > > reported as 'added' before.
> > I see this as a partial workaround for race condition. You 'fix' it if 
> > removal happens between inotify reporting the event and you returning it. 
> > You don't if removal happens after you push it to events. Therefore, the 
> > caller still needs to account for 'added' event being obsolete. I don't 
> > really see a purpose in trying to workaround the problem partially here if 
> > the caller still needs to account for it. Effectively, you're replacing one 
> > normal case and one corner case with one normal case and two corner cases.
> I was mainly pointing out that the client already has to be prepared for a 
> 'removed' event that does not have an associated 'added' event, regardless of 
> what this code is doing. Therefore a potential double 'removed' event doesn't 
> add complexity to the client.
> 
> Could you clarify how the code should handle the inability to get the mod 
> time ? Should it ignore the event ?
Given the code is supposed to wrap filesystem notification layer, I'd say it 
should pass the events unmodified and not double-guess what the client expects. 
The client needs to be prepared for non-atomicity of this anyway.


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

https://reviews.llvm.org/D58418



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

r354736 - Enable coroutines under -std=c++2a.

[Richard Smith via cfe-commits]

Author: rsmith
Date: Sat Feb 23 13:06:26 2019
New Revision: 354736

URL: http://llvm.org/viewvc/llvm-project?rev=354736&view=rev
Log:
Enable coroutines under -std=c++2a.

Modified:
cfe/trunk/include/clang/Basic/LangOptions.def
cfe/trunk/include/clang/Basic/TokenKinds.def
cfe/trunk/lib/Basic/IdentifierTable.cpp
cfe/trunk/lib/Basic/Module.cpp
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/lib/Frontend/InitPreprocessor.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/test/Lexer/cxx-features.cpp
cfe/trunk/test/Lexer/cxx2a_keyword_as_cxx17.cpp
cfe/trunk/www/cxx_status.html

Modified: cfe/trunk/include/clang/Basic/LangOptions.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=354736&r1=354735&r2=354736&view=diff
==
--- cfe/trunk/include/clang/Basic/LangOptions.def (original)
+++ cfe/trunk/include/clang/Basic/LangOptions.def Sat Feb 23 13:06:26 2019
@@ -136,7 +136,7 @@ LANGOPT(Freestanding, 1, 0, "freestandin
 LANGOPT(NoBuiltin , 1, 0, "disable builtin functions")
 LANGOPT(NoMathBuiltin , 1, 0, "disable math builtin functions")
 LANGOPT(GNUAsm, 1, 1, "GNU-style inline assembly")
-LANGOPT(CoroutinesTS  , 1, 0, "C++ coroutines TS")
+LANGOPT(Coroutines, 1, 0, "C++20 coroutines")
 LANGOPT(DllExportInlines  , 1, 1, "dllexported classes dllexport inline 
methods")
 LANGOPT(RelaxedTemplateTemplateArgs, 1, 0, "C++17 relaxed matching of template 
template arguments")
 

Modified: cfe/trunk/include/clang/Basic/TokenKinds.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/TokenKinds.def?rev=354736&r1=354735&r2=354736&view=diff
==
--- cfe/trunk/include/clang/Basic/TokenKinds.def (original)
+++ cfe/trunk/include/clang/Basic/TokenKinds.def Sat Feb 23 13:06:26 2019
@@ -32,6 +32,9 @@
 #ifndef CONCEPTS_KEYWORD
 #define CONCEPTS_KEYWORD(X) CXX2A_KEYWORD(X,KEYCONCEPTS)
 #endif
+#ifndef COROUTINES_KEYWORD
+#define COROUTINES_KEYWORD(X) CXX2A_KEYWORD(X,KEYCOROUTINES)
+#endif
 #ifndef MODULES_KEYWORD
 #define MODULES_KEYWORD(X) KEYWORD(X,KEYMODULES)
 #endif
@@ -254,8 +257,7 @@ PUNCTUATOR(caretcaret,"^^")
 //   KEYZVECTOR - This is a keyword for the System z vector extensions,
 //which are heavily based on AltiVec
 //   KEYBORLAND - This is a keyword if Borland extensions are enabled
-//   KEYCOROUTINES - This is a keyword if support for the C++ coroutines
-//   TS is enabled
+//   KEYCOROUTINES - This is a keyword if support for C++ coroutines is enabled
 //   BOOLSUPPORT - This is a keyword if 'bool' is a built-in type
 //   HALFSUPPORT - This is a keyword if 'half' is a built-in type
 //   WCHARSUPPORT - This is a keyword if 'wchar_t' is a built-in type
@@ -371,17 +373,17 @@ CXX11_KEYWORD(thread_local  , 0)
 CONCEPTS_KEYWORD(concept)
 CONCEPTS_KEYWORD(requires)
 
-// C++ coroutines TS keywords
-KEYWORD(co_await, KEYCOROUTINES)
-KEYWORD(co_return   , KEYCOROUTINES)
-KEYWORD(co_yield, KEYCOROUTINES)
+// C++2a / coroutines TS keywords
+COROUTINES_KEYWORD(co_await)
+COROUTINES_KEYWORD(co_return)
+COROUTINES_KEYWORD(co_yield)
 
 // C++ modules TS keywords
 MODULES_KEYWORD(module)
 MODULES_KEYWORD(import)
 
 // C++ char8_t proposal
-KEYWORD(char8_t , CHAR8SUPPORT)
+CXX2A_KEYWORD(char8_t   , CHAR8SUPPORT)
 
 // C11 Extension
 KEYWORD(_Float16, KEYALL)

Modified: cfe/trunk/lib/Basic/IdentifierTable.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/IdentifierTable.cpp?rev=354736&r1=354735&r2=354736&view=diff
==
--- cfe/trunk/lib/Basic/IdentifierTable.cpp (original)
+++ cfe/trunk/lib/Basic/IdentifierTable.cpp Sat Feb 23 13:06:26 2019
@@ -143,7 +143,7 @@ static KeywordStatus getKeywordStatus(co
   // in non-arc mode.
   if (LangOpts.ObjC && (Flags & KEYOBJC)) return KS_Enabled;
   if (LangOpts.ConceptsTS && (Flags & KEYCONCEPTS)) return KS_Enabled;
-  if (LangOpts.CoroutinesTS && (Flags & KEYCOROUTINES)) return KS_Enabled;
+  if (LangOpts.Coroutines && (Flags & KEYCOROUTINES)) return KS_Enabled;
   if (LangOpts.ModulesTS && (Flags & KEYMODULES)) return KS_Enabled;
   if (LangOpts.CPlusPlus && (Flags & KEYALLCXX)) return KS_Future;
   return KS_Disabled;

Modified: cfe/trunk/lib/Basic/Module.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Module.cpp?rev=354736&r1=354735&r2=354736&view=diff
==
--- cfe/trunk/lib/Basic/Module.cpp (original)
+++ cfe/trunk/lib/Basic/Module.cpp Sat Feb 23 13:06:26 2019
@@ -108,7 +108,7 @@ static bool hasFeature(StringRef Feature
   bool H

r354735 - [cxx_status] Update to match Kona motions.

[Richard Smith via cfe-commits]

Author: rsmith
Date: Sat Feb 23 13:06:25 2019
New Revision: 354735

URL: http://llvm.org/viewvc/llvm-project?rev=354735&view=rev
Log:
[cxx_status] Update to match Kona motions.

Modified:
cfe/trunk/www/cxx_status.html

Modified: cfe/trunk/www/cxx_status.html
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/www/cxx_status.html?rev=354735&r1=354734&r2=354735&view=diff
==
--- cfe/trunk/www/cxx_status.html (original)
+++ cfe/trunk/www/cxx_status.html Sat Feb 23 13:06:25 2019
@@ -104,10 +104,14 @@ with http://libcxx.llvm.org/";>l
   Clang 2.9
 
 
-  Initializer lists
+  Initializer lists
   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2672.htm";>N2672
   Clang 3.1
 
+   
+http://wg21.link/p1009r2";>P1009R2 (DR)
+No
+  
 
   Static assertions
   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2004/n1720.html";>N1720
@@ -275,10 +279,14 @@ with http://libcxx.llvm.org/";>l
   Clang 3.0
 
 
-  Defaulted functions
+  Defaulted functions
   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2346.htm";>N2346
   Clang 3.0
 
+   
+http://wg21.link/p1286r2";>P1286R2 (DR)
+No
+  
 
   Deleted functions
   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2346.htm";>N2346
@@ -893,7 +901,7 @@ as the draft C++2a standard evolves.
   SVN
 
 
-  Consistent comparison (operator<=>)
+  Consistent comparison (operator<=>)
   http://wg21.link/p0515r3";>P0515R3
   Partial
 
@@ -903,6 +911,10 @@ as the draft C++2a standard evolves.

 http://wg21.link/p1120r0";>P1120R0
   
+   
+http://wg21.link/p1185r2";>P1185R2
+No
+  
 
   Access checking on specializations
   http://wg21.link/p0692r1";>P0692R1
@@ -972,13 +984,16 @@ as the draft C++2a standard evolves.
   SVN
 
 
-  Contracts
+  Contracts
   http://wg21.link/p0542r5";>P0542R5
-  No
+  No
 

 http://wg21.link/p1289r1";>P1289R1
   
+   
+http://wg21.link/p1323r2";>P1323R2
+  
 
   Feature test macros
   http://wg21.link/p0941r2";>P0941R2
@@ -1015,6 +1030,38 @@ as the draft C++2a standard evolves.
   http://wg21.link/p1094r2";>P1094R2
   SVN
 
+
+
+  Structured binding extensions
+  http://wg21.link/p1091r3";>P1091R3
+  No
+
+  
+http://wg21.link/p1381r1";>P1381R1
+  
+
+  Stronger Unicode requirements
+  http://wg21.link/p1041r4";>P1041R4
+  Yes
+
+  
+http://wg21.link/p1139r2";>P1139R2
+  
+
+  Parenthesized initialization of aggregates
+  http://wg21.link/p0960r3";>P0960R3
+  No
+
+
+  Modules
+  http://wg21.link/p1103r3";>P1103R3
+  Partial (-fmodules, 
-fmodules-ts)
+
+
+  Coroutines
+  http://wg21.link/p0912r5";>P0912R5
+  Partial 
(-fcoroutines-ts)
+
 
 
 
@@ -1108,12 +1155,16 @@ and library features that are not part o
 
 
   
-  [DRAFT TS] Coroutines
-  https://isocpp.org/files/papers/N4663.pdf";>N4663
+  [TS] Coroutines
+  https://isocpp.org/files/papers/N4663.pdf";>N4663
   -fcoroutines-ts-stdlib=libc++
   Clang 5
 
 
+  -std=c++2a-stdlib=libc++
+  Superseded by P0912R5
+
+
   [TS] Library Fundamentals, Version 1 (invocation type traits)
   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4480.html";>N4480
   N/A
@@ -1129,7 +1180,7 @@ and library features that are not part o
   [TS] Modules
   http://wg21.link/n4720";>N4720
   -fmodules-ts
-  WIP
+  Superseded by P1103R3
 
 
   [DRAFT TS] Reflection


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

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > akyrtzi wrote:
> > > > mgorny wrote:
> > > > > jkorous wrote:
> > > > > > mgorny wrote:
> > > > > > > Why? I suppose this deserves a comment.
> > > > > > I'll add this comment:
> > > > > > 
> > > > > > // The file might have been removed just after we received the 
> > > > > > event.
> > > > > Wouldn't that cause removals to be reported twice?
> > > > Not quite sure if it can happen in practice but I'd suggest to accept 
> > > > this as potential occurrence and add it to documentation ("a 'removed' 
> > > > event may be reported twice). I think it is better to handle a definite 
> > > > "fact" (the file doesn't exist) than ignore it and assume the 'removed' 
> > > > event will eventually show up, or try to eliminate the double reporting 
> > > > which would over-complicate the implementation.
> > > > 
> > > > After all, if inotify() is not 100% reliable then there's already the 
> > > > possibility that you'll get a 'removed' event for a file that was not 
> > > > reported as 'added' before.
> > > I see this as a partial workaround for race condition. You 'fix' it if 
> > > removal happens between inotify reporting the event and you returning it. 
> > > You don't if removal happens after you push it to events. Therefore, the 
> > > caller still needs to account for 'added' event being obsolete. I don't 
> > > really see a purpose in trying to workaround the problem partially here 
> > > if the caller still needs to account for it. Effectively, you're 
> > > replacing one normal case and one corner case with one normal case and 
> > > two corner cases.
> > I was mainly pointing out that the client already has to be prepared for a 
> > 'removed' event that does not have an associated 'added' event, regardless 
> > of what this code is doing. Therefore a potential double 'removed' event 
> > doesn't add complexity to the client.
> > 
> > Could you clarify how the code should handle the inability to get the mod 
> > time ? Should it ignore the event ?
> Given the code is supposed to wrap filesystem notification layer, I'd say it 
> should pass the events unmodified and not double-guess what the client 
> expects. The client needs to be prepared for non-atomicity of this anyway.
So it should report an 'added' event but with optional mod-time, essentially 
reporting that something was added that doesn't exist ?
I'd prefer not to do that because it complicates the client without any real 
benefit. It was great that you pointed out this part of the code but I'd 
recommend that if the file is gone we should ignore the 'added' event, instead 
of complicating the API for a corner case.


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

https://reviews.llvm.org/D58418



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

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

akyrtzi wrote:
> mgorny wrote:
> > akyrtzi wrote:
> > > mgorny wrote:
> > > > akyrtzi wrote:
> > > > > mgorny wrote:
> > > > > > jkorous wrote:
> > > > > > > mgorny wrote:
> > > > > > > > Why? I suppose this deserves a comment.
> > > > > > > I'll add this comment:
> > > > > > > 
> > > > > > > // The file might have been removed just after we received the 
> > > > > > > event.
> > > > > > Wouldn't that cause removals to be reported twice?
> > > > > Not quite sure if it can happen in practice but I'd suggest to accept 
> > > > > this as potential occurrence and add it to documentation ("a 
> > > > > 'removed' event may be reported twice). I think it is better to 
> > > > > handle a definite "fact" (the file doesn't exist) than ignore it and 
> > > > > assume the 'removed' event will eventually show up, or try to 
> > > > > eliminate the double reporting which would over-complicate the 
> > > > > implementation.
> > > > > 
> > > > > After all, if inotify() is not 100% reliable then there's already the 
> > > > > possibility that you'll get a 'removed' event for a file that was not 
> > > > > reported as 'added' before.
> > > > I see this as a partial workaround for race condition. You 'fix' it if 
> > > > removal happens between inotify reporting the event and you returning 
> > > > it. You don't if removal happens after you push it to events. 
> > > > Therefore, the caller still needs to account for 'added' event being 
> > > > obsolete. I don't really see a purpose in trying to workaround the 
> > > > problem partially here if the caller still needs to account for it. 
> > > > Effectively, you're replacing one normal case and one corner case with 
> > > > one normal case and two corner cases.
> > > I was mainly pointing out that the client already has to be prepared for 
> > > a 'removed' event that does not have an associated 'added' event, 
> > > regardless of what this code is doing. Therefore a potential double 
> > > 'removed' event doesn't add complexity to the client.
> > > 
> > > Could you clarify how the code should handle the inability to get the mod 
> > > time ? Should it ignore the event ?
> > Given the code is supposed to wrap filesystem notification layer, I'd say 
> > it should pass the events unmodified and not double-guess what the client 
> > expects. The client needs to be prepared for non-atomicity of this anyway.
> So it should report an 'added' event but with optional mod-time, essentially 
> reporting that something was added that doesn't exist ?
> I'd prefer not to do that because it complicates the client without any real 
> benefit. It was great that you pointed out this part of the code but I'd 
> recommend that if the file is gone we should ignore the 'added' event, 
> instead of complicating the API for a corner case.
Except that is technically impossible to avoid reporting something that doesn't 
exist because it can be removed just after you check for it. So the client 
needs to *always* support it, otherwise it's fragile to race conditions.

This extra check just covers the short period (-> 0) between reporting and 
checking. It's needless complexity that doesn't solve the problem. If it does 
anything, then it gives you false security that you've solved the problem when 
actually the file may still disappear 1 ns after you've checked that it existed.


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

https://reviews.llvm.org/D58418



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

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added inline comments.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:135
+if (!statusOpt.hasValue())
+  K = DirectoryWatcher::EventKind::Removed;
+  }

mgorny wrote:
> akyrtzi wrote:
> > mgorny wrote:
> > > akyrtzi wrote:
> > > > mgorny wrote:
> > > > > akyrtzi wrote:
> > > > > > mgorny wrote:
> > > > > > > jkorous wrote:
> > > > > > > > mgorny wrote:
> > > > > > > > > Why? I suppose this deserves a comment.
> > > > > > > > I'll add this comment:
> > > > > > > > 
> > > > > > > > // The file might have been removed just after we received the 
> > > > > > > > event.
> > > > > > > Wouldn't that cause removals to be reported twice?
> > > > > > Not quite sure if it can happen in practice but I'd suggest to 
> > > > > > accept this as potential occurrence and add it to documentation ("a 
> > > > > > 'removed' event may be reported twice). I think it is better to 
> > > > > > handle a definite "fact" (the file doesn't exist) than ignore it 
> > > > > > and assume the 'removed' event will eventually show up, or try to 
> > > > > > eliminate the double reporting which would over-complicate the 
> > > > > > implementation.
> > > > > > 
> > > > > > After all, if inotify() is not 100% reliable then there's already 
> > > > > > the possibility that you'll get a 'removed' event for a file that 
> > > > > > was not reported as 'added' before.
> > > > > I see this as a partial workaround for race condition. You 'fix' it 
> > > > > if removal happens between inotify reporting the event and you 
> > > > > returning it. You don't if removal happens after you push it to 
> > > > > events. Therefore, the caller still needs to account for 'added' 
> > > > > event being obsolete. I don't really see a purpose in trying to 
> > > > > workaround the problem partially here if the caller still needs to 
> > > > > account for it. Effectively, you're replacing one normal case and one 
> > > > > corner case with one normal case and two corner cases.
> > > > I was mainly pointing out that the client already has to be prepared 
> > > > for a 'removed' event that does not have an associated 'added' event, 
> > > > regardless of what this code is doing. Therefore a potential double 
> > > > 'removed' event doesn't add complexity to the client.
> > > > 
> > > > Could you clarify how the code should handle the inability to get the 
> > > > mod time ? Should it ignore the event ?
> > > Given the code is supposed to wrap filesystem notification layer, I'd say 
> > > it should pass the events unmodified and not double-guess what the client 
> > > expects. The client needs to be prepared for non-atomicity of this anyway.
> > So it should report an 'added' event but with optional mod-time, 
> > essentially reporting that something was added that doesn't exist ?
> > I'd prefer not to do that because it complicates the client without any 
> > real benefit. It was great that you pointed out this part of the code but 
> > I'd recommend that if the file is gone we should ignore the 'added' event, 
> > instead of complicating the API for a corner case.
> Except that is technically impossible to avoid reporting something that 
> doesn't exist because it can be removed just after you check for it. So the 
> client needs to *always* support it, otherwise it's fragile to race 
> conditions.
> 
> This extra check just covers the short period (-> 0) between reporting and 
> checking. It's needless complexity that doesn't solve the problem. If it does 
> anything, then it gives you false security that you've solved the problem 
> when actually the file may still disappear 1 ns after you've checked that it 
> existed.
Ok, that's fair, @jkorous I'm fine with removing the mod-time from the 
DirectoryWatcher API. We can get and report the mod-time at the index-store 
library layer.


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

https://reviews.llvm.org/D58418



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

[PATCH] D58586: Clear TimerGroup to avoid redundant profile results

[Takafumi Kubota via Phabricator via cfe-commits]

tk1012 created this revision.
tk1012 added a project: clang.
Herald added a subscriber: cfe-commits.

This patch clears out all timers just after printing all timers.


Repository:
  rC Clang

https://reviews.llvm.org/D58586

Files:
  clang/tools/driver/cc1_main.cpp
  clang/tools/driver/cc1as_main.cpp


Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -590,6 +590,7 @@
   // If any timers were active but haven't been destroyed yet, print their
   // results now.
   TimerGroup::printAll(errs());
+  TimerGroup::clearAll();
 
   return !!Failed;
 }
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -220,6 +220,7 @@
   // If any timers were active but haven't been destroyed yet, print their
   // results now.  This happens in -disable-free mode.
   llvm::TimerGroup::printAll(llvm::errs());
+  llvm::TimerGroup::clearAll();
 
   // Our error handler depends on the Diagnostics object, which we're
   // potentially about to delete. Uninstall the handler now so that any


Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -590,6 +590,7 @@
   // If any timers were active but haven't been destroyed yet, print their
   // results now.
   TimerGroup::printAll(errs());
+  TimerGroup::clearAll();
 
   return !!Failed;
 }
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -220,6 +220,7 @@
   // If any timers were active but haven't been destroyed yet, print their
   // results now.  This happens in -disable-free mode.
   llvm::TimerGroup::printAll(llvm::errs());
+  llvm::TimerGroup::clearAll();
 
   // Our error handler depends on the Diagnostics object, which we're
   // potentially about to delete. Uninstall the handler now so that any
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r354741 - [NFC] Minor coding style (indent) fix.

[Michael Liao via cfe-commits]

Author: hliao
Date: Sat Feb 23 19:07:32 2019
New Revision: 354741

URL: http://llvm.org/viewvc/llvm-project?rev=354741&view=rev
Log:
[NFC] Minor coding style (indent) fix.

Modified:
cfe/trunk/lib/AST/Type.cpp

Modified: cfe/trunk/lib/AST/Type.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Type.cpp?rev=354741&r1=354740&r2=354741&view=diff
==
--- cfe/trunk/lib/AST/Type.cpp (original)
+++ cfe/trunk/lib/AST/Type.cpp Sat Feb 23 19:07:32 2019
@@ -1182,7 +1182,7 @@ struct SubstObjCTypeArgsVisitor
 
   // Rebuild object pointer type.
   return Ctx.getObjCObjectPointerType(resultTy);
-  }
+}
 }
 llvm_unreachable("Unexpected ObjCSubstitutionContext!");
   }


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

r354742 - Typo: s/CHCCK/CHECK

[Michael Liao via cfe-commits]

Author: hliao
Date: Sat Feb 23 19:10:14 2019
New Revision: 354742

URL: http://llvm.org/viewvc/llvm-project?rev=354742&view=rev
Log:
Typo: s/CHCCK/CHECK

Modified:
cfe/trunk/test/CodeGenCXX/pragma-loop-safety.cpp

Modified: cfe/trunk/test/CodeGenCXX/pragma-loop-safety.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/pragma-loop-safety.cpp?rev=354742&r1=354741&r2=354742&view=diff
==
--- cfe/trunk/test/CodeGenCXX/pragma-loop-safety.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/pragma-loop-safety.cpp Sat Feb 23 19:10:14 2019
@@ -49,7 +49,7 @@ void interleave_test(int *List, int Leng
 // CHECK: ![[ACCESS_GROUP_2]] = distinct !{}
 // CHECK: ![[LOOP1_HINTS]] = distinct !{![[LOOP1_HINTS]], 
![[INTERLEAVE_1:[0-9]+]], ![[INTENABLE_1:[0-9]+]], ![[UNROLL_DISABLE:[0-9]+]], 
![[PARALLEL_ACCESSES_7:[0-9]+]]}
 // CHECK: ![[INTERLEAVE_1]] = !{!"llvm.loop.interleave.count", i32 1}
-// CHCCK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[PARALLEL_ACCESSES_7]] = !{!"llvm.loop.parallel_accesses", 
![[ACCESS_GROUP_2]]}
 // CHECK: ![[ACCESS_GROUP_8]] = distinct !{}


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