[PATCH] D75723: [X86] Enable intrinsics _BitScan*

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/Headers/ia32intrin.h:421
+if (__c != 0) {
\
+  *(a) = (unsigned)__bsfd(__c);
\
+  __d = 1; 
\

skan wrote:
> craig.topper wrote:
> > Should (a) have a cast to (unsigned *)?
> No. Using a cast here may cause pointer aliasing problem. 
> 
> https://stackoverflow.com/questions/98650/what-is-the-strict-aliasing-rule
> 
> If a is not `unsigned *`, letting the compiler do auto integer conversion for 
> `*(a)` is the correct way, from my perspective.
But if we implemented this as an inline function instead of a macro wouldn't 
the argument have been an unsigned *?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75723



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

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

I'm done, Aaron, Quuxplusone, do you have any comments?


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

https://reviews.llvm.org/D74692



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

Re: [clang-tools-extra] 54928ba - [clang-tidy] Use more widely available headers for protability-restrict-system-includes-check's test

[Roman Lebedev via cfe-commits]

On Wed, Mar 11, 2020 at 2:54 AM Paula Toth via cfe-commits
 wrote:
>
>
> Author: Paula Toth
> Date: 2020-03-10T16:54:11-07:00
> New Revision: 54928ba0ec8355edbbdb31c7b01bb45eb2d384b6
>
> URL: 
> https://github.com/llvm/llvm-project/commit/54928ba0ec8355edbbdb31c7b01bb45eb2d384b6
> DIFF: 
> https://github.com/llvm/llvm-project/commit/54928ba0ec8355edbbdb31c7b01bb45eb2d384b6.diff
>
> LOG: [clang-tidy] Use more widely available headers for 
> protability-restrict-system-includes-check's test
>
> Added:
>
>
> Modified:
> 
> clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
> 
> clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
> 
> clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-glob.cpp
>
> Removed:
>
>
>
> 
> diff  --git 
> a/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
>  
> b/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
> index faab977ad340..e3a9376a8638 100644
> --- 
> a/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
> +++ 
> b/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-allow.cpp
> @@ -1,9 +1,9 @@
>  // RUN: %check_clang_tidy %s portability-restrict-system-includes %t \
> -// RUN: -- -config="{CheckOptions: [{key: 
> portability-restrict-system-includes.Includes, value: '*,-stdio.h'}]}"
> +// RUN: -- -config="{CheckOptions: [{key: 
> portability-restrict-system-includes.Includes, value: '*,-stddef.h'}]}"
>
> -// Test block-list functionality: allow all but stdio.h.
> +// Test block-list functionality: allow all but stddef.h.
>
> -#include 
> -// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stdio.h not 
> allowed
> -#include 
> -#include 
> +#include 
> +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stddef.h not 
> allowed
> +#include 
> +#include 
In this test, are these actual system headers, or are they mocked?
Tests should not be using system headers.

> diff  --git 
> a/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
>  
> b/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
> index 58c1d4396ee5..f73cbbfc816d 100644
> --- 
> a/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
> +++ 
> b/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-disallow.cpp
> @@ -1,10 +1,10 @@
>  // RUN: %check_clang_tidy %s portability-restrict-system-includes %t \
> -// RUN: -- -config="{CheckOptions: [{key: 
> portability-restrict-system-includes.Includes, value: '-*,stdio.h'}]}"
> +// RUN: -- -config="{CheckOptions: [{key: 
> portability-restrict-system-includes.Includes, value: '-*,stddef.h'}]}"
>
> -// Test allow-list functionality: disallow all but stdio.h.
> +// Test allow-list functionality: disallow all but stddef.h.
>
> -#include 
> -#include 
> -// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stdlib.h not 
> allowed
> -#include 
> -// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include string.h not 
> allowed
> +#include 
> +#include 
> +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include stdint.h not 
> allowed
> +#include 
> +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include float.h not 
> allowed
>
> diff  --git 
> a/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-glob.cpp
>  
> b/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-glob.cpp
> index ccde9438d93c..bee7ec73438c 100644
> --- 
> a/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-glob.cpp
> +++ 
> b/clang-tools-extra/test/clang-tidy/checkers/portability-restrict-system-includes-glob.cpp
> @@ -4,7 +4,7 @@
>  // Test glob functionality: disallow all headers except those that match
>  // pattern "std*.h".
>
> -#include 
> -#include 
> -#include 
> -// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include string.h not 
> allowed
> +#include 
> +#include 
> +#include 
> +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: system include float.h not 
> allowed
Roman

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

[PATCH] D75723: [X86] Enable intrinsics _BitScan*

[Kan Shengchen via Phabricator via cfe-commits]

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



Comment at: clang/lib/Headers/ia32intrin.h:421
+if (__c != 0) {
\
+  *(a) = (unsigned)__bsfd(__c);
\
+  __d = 1; 
\

craig.topper wrote:
> skan wrote:
> > craig.topper wrote:
> > > Should (a) have a cast to (unsigned *)?
> > No. Using a cast here may cause pointer aliasing problem. 
> > 
> > https://stackoverflow.com/questions/98650/what-is-the-strict-aliasing-rule
> > 
> > If a is not `unsigned *`, letting the compiler do auto integer conversion 
> > for `*(a)` is the correct way, from my perspective.
> But if we implemented this as an inline function instead of a macro wouldn't 
> the argument have been an unsigned *?
Yes, it would have been.  But I don't known the motivation to use a cast 
`(unsigned *)` here,  is there any case using cast here works correctly while 
not using cast here fails?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75723



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

[PATCH] D75922: [ASTImporter] Compare the DC of the underlying type of a FriendDecl

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3679
+if (FromFriendDC) { // The underlying friend type is not dependent.
+  ExpectedDecl FriendDCDeclOrErr = import(cast(*FromFriendDC));
+  if (!FriendDCDeclOrErr)

Computing `FriendDC` can be moved out of the loop.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:4035
+  )";
+  Decl *FromTu = getTuDecl(Code, Lang_CXX, "from.cc");
+

Use `ToTU` and `FromTU` as is in other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75922



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

[PATCH] D75901: [clang-tidy] misc-unconventional-assign-operator suggest to use rvalue references in C++03 mode

[Nathan James via Phabricator via cfe-commits]

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

LGTM
For the record `check-clang-tools` is sufficient for testing all clang tidy 
checks


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

https://reviews.llvm.org/D75901



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 2 inline comments as done.
balazske added a comment.

I do not understand totally this remove-of-AnyError thing. If handling the 
`AnyError` will be removed the code remains still not testable. Because none of 
the possible failing file operations are modeled in this revision. A test with 
code like `if (feof(F)) {` can not work because `feof(F)` is never true without 
a previously failed operation. (We decide not at the moment of calling `feof` 
that `feof` will return true, it is decided at a previously failed operation, 
together with the return value of that operation.)




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:44
+OtherError, /// Other (non-EOF) error (`ferror` is true).
+AnyError/// EofError or OtherError, used if exact error is unknown.
+  } ErrorState = NoError;

Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > When do we know that the stream is in an error state, but not precisely 
> > > know what that error is within the context of this patch?  `fseek` would 
> > > indeed introduce such a state, as described in D75356#inline-689287, but 
> > > that should introduce its own error `ErrorKind`. I still don't really get 
> > > why we would ever need `AnyError`.
> > This is the problem is the change is too small: The next part of it 
> > introduces functions where the new error flags are used. In this patch the 
> > AnyError feature is not used (the feof and ferror read it but nothing sets 
> > it).
> Okay, after your comment in the other revision, I see how such a kind should 
> be left, and the bug report should rather be enriched by `NoteTags`. With 
> that said, `AnyError` is still a bit confusing as a name -- how abour 
> `UnknownError`?
I want it to rename to `FEofOrFErrorError`, this tells exactly what it is 
(`UnknownError` can be still thought as a third error type). For these 2 error 
types this naming is not too long, and new error types are not likely to appear 
here later.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:51
+
+  bool isNoError() const { return ErrorState == NoError; }
+  bool isEofError() const { return ErrorState == EofError; }

Szelethus wrote:
> If a stream's opening failed, its still an error, yet this getter would 
> return true for it. I think this is yet another reason to just merge the 2 
> enums :)
But as the comment says, the error state is applicable only in the opened 
state. If not opened, we may not call the "error" functions at all (assertion 
can be added). Anyway it is possible to merge the enums, then the `isOpened` 
will be a bit difficult (true if state is one of `OpenedNoError`, 
`OpenedFErrorError`, `OpenedFEofError`, `OpenedFEofOrFErrorError`). Logically 
it is better to see that the state is an aggregation of two states, an 
"openedness" and an error state. Probably this does not matter much here 
because it is unlikely that new states will appear that make the code more 
complex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D75591: [OpenMP] Add firstprivate as a default data-sharing attribute to clang

[Atmn Patel via Phabricator via cfe-commits]

atmnpatel updated this revision to Diff 249570.
atmnpatel added a comment.

Modified the unit test for CodeGen of default(firstprivate) to more accurately 
reflect the IR.

>From the perspective of the AST, the variables that are firstprivate from a 
>default clause are managed in a way that is similar to how variables in a task 
>are marked firstprivate. This is responsible for the discrepancy between the 
>IR for 'default(firstprivate)' and 'default(none) firstprivate(...)'. When a 
>firstprivate clause is handled, it has an explicit list where it stores all 
>private copies that it has constructed for each variable because it has access 
>to an explicit list of variables for which it pre-emptively creates new copies 
>for. For a default clause, if a new copy of a variable needs to be created in 
>a manner idential to 'firstprivate(...)', it would require a complete 
>restructuring of how the default clause is handled because before the 
>constructor for the default clause can be called, we would need to iterate 
>over the contents of the region in order to find references to variables that 
>would need to become firstprivate to create and store copies.

The way that I see it, is that this would be require a much more substantial 
change (than the current proposed implementation) to the handling of the 
default clause - the construction of the default clause node in the AST would 
need to be postponed until after the contents of the openmp region had been 
placed into an AST and the nodes visited in order to find the relevant 
variables. It is my belief that this would lead tto a more convoluted 
implementation whose behavior is identical to the current one.

I can confirm that in the IR it does not behave as an explicit firstprivate 
clause for the variables it marks as firstprivate but I don't understand enough 
to know why this causes a functional difference because the variables are 
handled as firstprivate in the same way that task handles them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75591

Files:
  clang-tools-extra/docs/clang-tidy/checks/openmp-use-default-none.rst
  clang-tools-extra/test/clang-tidy/checkers/openmp-use-default-none.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/distribute_parallel_for_default_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_default_messages.cpp
  clang/test/OpenMP/parallel_default_messages.cpp
  clang/test/OpenMP/parallel_for_default_messages.cpp
  clang/test/OpenMP/parallel_for_simd_default_messages.cpp
  clang/test/OpenMP/parallel_master_codegen.cpp
  clang/test/OpenMP/parallel_master_default_messages.cpp
  clang/test/OpenMP/parallel_sections_default_messages.cpp
  clang/test/OpenMP/target_parallel_default_messages.cpp
  clang/test/OpenMP/target_parallel_for_default_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_default_messages.cpp
  clang/test/OpenMP/target_teams_default_messages.cpp
  clang/test/OpenMP/target_teams_distribute_default_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_default_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_default_messages.cpp
  clang/test/OpenMP/task_default_messages.cpp
  clang/test/OpenMP/task_messages.cpp
  clang/test/OpenMP/teams_default_messages.cpp
  clang/test/OpenMP/teams_distribute_default_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_default_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_default_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_default_messages.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

Index: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
===
--- llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
+++ llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
@@ -381,6 +381,7 @@
 
 __OMP_DEFAULT_KIND(none)
 __OMP_DEFAULT_KIND(shared)
+__OMP_DEFAULT_KIND(firstprivate)
 __OMP_DEFAULT_KIND(unknown)
 
 #undef __OMP_DEFAULT_KIND
Index: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -1843,11 +1843,18 @@
   EXPECT_TRUE(matchesWithOpenMP(Source3, Matcher));
 
   const std::string Source4 = R"(
+void x() {
+#pragma omp parallel default(firstprivate)
+;
+})";
+  EXPECT_TRUE(matchesWithOpenMP(Source4, Matcher));
+
+  const std::string Source5 = R"(
 void x(int x) {
 #pragma omp parallel num_threads(x)
 ;
 })";
-  EXPECT_TRUE(notMatchesWithOpenMP(Source4, Matcher));
+  EXPECT_TRUE(notMatchesWithOpenMP(Source5, M

[PATCH] D75406: Avoid including FileManager.h from SourceManager.h

[Hans Wennborg via Phabricator via cfe-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75406



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

[PATCH] D75844: [clang] Set begin loc on GNU attribute parsed attrs

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 249574.
tbaeder added a comment.

Updated according to Aaron's comments. I'm not sure about changing the 
`DeclSpec` member and moving `ParsedAttributesWithRange` to ParsedAttr.h.

Tests included.


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

https://reviews.llvm.org/D75844

Files:
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/test/AST/sourceranges.cpp
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp

Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -107,6 +107,11 @@
   break;
 case 25: // no warning here, there's no fall-through
   break;
+case 26:
+  return 0;
+__attribute__((fallthrough)); // expected-warning{{fallthrough annotation in unreachable code}}
+case 27:
+  break;
   }
 
   return n;
Index: clang/test/AST/sourceranges.cpp
===
--- clang/test/AST/sourceranges.cpp
+++ clang/test/AST/sourceranges.cpp
@@ -108,6 +108,25 @@
   }
 }
 
+// CHECK-1Z: NamespaceDecl {{.*}} attributed_case
+namespace attributed_case {
+  void f(int n) {
+switch(n) {
+  case 0:
+n--;
+// CHECK: AttributedStmt {{.*}} 
+// CHECK: FallThroughAttr {{.*}} 
+__attribute__((fallthrough))
+// CHECK: FallThroughAttr {{.*}} 
+  __attribute__((fallthrough))
+  ;
+  case 1:
+n++;
+break;
+}
+  }
+}
+
 #if __cplusplus >= 201703L
 // CHECK-1Z: FunctionDecl {{.*}} construct_with_init_list
 std::map construct_with_init_list() {
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -2493,7 +2493,7 @@
   Braces.consumeClose();
 }
 
-bool Parser::ParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs) {
+bool Parser::ParseOpenCLUnrollHintAttribute(ParsedAttributesWithRange &Attrs) {
   MaybeParseGNUAttributes(Attrs);
 
   if (Attrs.empty())
Index: clang/lib/Parse/ParseObjc.cpp
===
--- clang/lib/Parse/ParseObjc.cpp
+++ clang/lib/Parse/ParseObjc.cpp
@@ -25,7 +25,7 @@
 
 /// Skips attributes after an Objective-C @ directive. Emits a diagnostic.
 void Parser::MaybeSkipAttributes(tok::ObjCKeywordKind Kind) {
-  ParsedAttributes attrs(AttrFactory);
+  ParsedAttributesWithRange attrs(AttrFactory);
   if (Tok.is(tok::kw___attribute)) {
 if (Kind == tok::objc_interface || Kind == tok::objc_protocol)
   Diag(Tok, diag::err_objc_postfix_attribute_hint)
@@ -1348,7 +1348,7 @@
nullptr);
 
   // If attributes exist before the method, parse them.
-  ParsedAttributes methodAttrs(AttrFactory);
+  ParsedAttributesWithRange methodAttrs(AttrFactory);
   if (getLangOpts().ObjC)
 MaybeParseGNUAttributes(methodAttrs);
   MaybeParseCXX11Attributes(methodAttrs);
@@ -1397,7 +1397,7 @@
 
   AttributePool allParamAttrs(AttrFactory);
   while (1) {
-ParsedAttributes paramAttrs(AttrFactory);
+ParsedAttributesWithRange paramAttrs(AttrFactory);
 Sema::ObjCArgInfo ArgInfo;
 
 // Each iteration parses a single keyword argument.
Index: clang/lib/Parse/ParseExprCXX.cpp
===
--- clang/lib/Parse/ParseExprCXX.cpp
+++ clang/lib/Parse/ParseExprCXX.cpp
@@ -1234,7 +1234,7 @@
   TemplateParameterDepthRAII CurTemplateDepthTracker(TemplateParameterDepth);
   Actions.PushLambdaScope();
 
-  ParsedAttributes Attr(AttrFactory);
+  ParsedAttributesWithRange Attr(AttrFactory);
   SourceLocation DeclLoc = Tok.getLocation();
   if (getLangOpts().CUDA) {
 // In CUDA code, GNU attributes are allowed to appear immediately after the
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -516,7 +516,7 @@
 Decl *Parser::ParseUsingDirective(DeclaratorContext Context,
   SourceLocation UsingLoc,
   SourceLocation &DeclEnd,
-  ParsedAttributes &attrs) {
+  ParsedAttributesWithRange &attrs) {
   assert(Tok.is(tok::kw_namespace) && "Not 'namespace' token");
 
   // Eat 'namespace'.
@@ -3050,7 +3050,7 @@
   T.skipToEnd();
 
   // Parse and discard any trailing attributes.
-  ParsedAttributes Attrs(AttrFactory);
+  ParsedAttributesWithRange Attrs(AttrFactory);
   if (Tok.is(to

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro added a comment.

In D73534#1916291 , @djtodoro wrote:

> In D73534#1915048 , @mstorsjo wrote:
>
> > This broke compiling for mingw, repro.c:
> >
> >   a(short);
> >   b() { a(1); }
> >
> >
> > `clang -target x86_64-w64-mingw32 -c repro.c -g -O2`, which gives 
> > `Assertion '!MI.isMoveImmediate() && "Unexpected MoveImm instruction"' 
> > failed.`
>
>
> Thanks for reporting this! Since this is the case of the `X86::MOV16ri` the 
> D75326  will solve this issue.


The alternative is D75974 .


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

https://reviews.llvm.org/D73534



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

[PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro added a comment.

In D73534#1915048 , @mstorsjo wrote:

> This broke compiling for mingw, repro.c:
>
>   a(short);
>   b() { a(1); }
>
>
> `clang -target x86_64-w64-mingw32 -c repro.c -g -O2`, which gives `Assertion 
> '!MI.isMoveImmediate() && "Unexpected MoveImm instruction"' failed.`


Thanks for reporting this! Since this is the case of the `X86::MOV16ri` the 
D75326  will solve this issue.


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

https://reviews.llvm.org/D73534



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45
+  enum KindTy {
+Opened, /// Stream is opened.
+Closed, /// Closed stream (an invalid stream pointer after it was closed).
+OpenFailed /// The last open operation has failed.
+  } State;
+
+  /// The error state of a stream.

Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Hmm, now that I think of it, could we just merge these 2 enums? Also, I 
> > > fear that indexers would accidentally assign the comment to the enum 
> > > after the comma:
> > > 
> > > ```lang=cpp
> > > Opened, /// Stream is opened.
> > > Closed, /// Closed stream (an invalid stream pointer after it was 
> > > closed).
> > > OpenFailed /// The last open operation has failed.
> > > ```
> > > ` /// Stream is opened` might be assigned to `Closed`. How about this:
> > > ```lang=cpp
> > > /// Stream is opened.
> > > Opened,
> > > /// Closed stream (an invalid stream pointer after it was closed).
> > > Closed,
> > > /// The last open operation has failed.
> > > OpenFailed
> > > ```
> > Probably these can be merged, it is used for a stream that is in an 
> > indeterminate state after failed `freopen`, but practically it is handled 
> > the same way as a closed stream. But this change would be done in another 
> > revision.
> I meant to merge the two enums (`StreamState` and `ErrorKindTy`) and the 
> fields associated with them (`State` and `ErrorState`). We could however 
> merge `Closed` and `OpenFailed`, granted that we put a `NoteTag` to 
> `evalFreopen`. But I agree, that should be another revision's topic :)
Since you mentioned that ErrorState is only applicable to Open streams, I am 
also +1 on merging the enums. These two states are not orthogonal, no reason 
for them to be separate.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
+StreamSym, StreamState::getOpenedWithOtherError());
+C.addTransition(StateEof);
+C.addTransition(StateOther);

As you probably know we are splitting the execution paths here. This will 
potentially result in doubling the analysis tome for a function for each `feof` 
call. Since state splits can be really bad for performance we need good 
justification for each of them.
So the questions are:
* Is it really important to differentiate between eof and other error or is it 
possible to just have an any error state?
* Do we find more bugs in real world code that justifies these state splits?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D65994: Extended FPOptions with new attributes

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This patch increased the used size of BinaryOperator by 5 bits.
Those bits were all padding, but now BinaryOperatorBitfields is completely full 
at 32 bits and we can't add any new bits to Stmt without increasing 
BinaryOperator by 8 bytes. (See D75443  and 
D54526  for the optimization this would 
revert).

To squeeze in the new bit I'm planning to suggest squeezing getInt() to 7 bits 
(it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not really 
familiar with this domain - if many of the 90 states are not possible it'd 
probably be useful to have some more bits back :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65994



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

[PATCH] D75861: [SVE] Generate overloaded functions for ACLE intrinsics.

[Sjoerd Meijer via Phabricator via cfe-commits]

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

With the nit addressed, this LGTM




Comment at: clang/utils/TableGen/SveEmitter.cpp:634
 " *\n"
 " * Permission is hereby granted, free of charge, to any person "
 "obtaining "

SjoerdMeijer wrote:
> Unrelated to this change, I just spotted it because it looked a bit messy, 
> but is this the old copyright header? While we are at it, replace this with 
> the new one? Looks like e.g. in arm_mve.h, we do use the new one. 
Let's ignore this for now, we can fix this later if needed.


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

https://reviews.llvm.org/D75861



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

[PATCH] D75922: [ASTImporter] Compare the DC of the underlying type of a FriendDecl

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D75922#1915918 , @shafik wrote:

> I believe that your main example violates basic.scope.class p2 
> :
>
> > A name N used in a class S shall refer to the same declaration in its 
> > context and when re-evaluated in the completed scope of S.
> >  No diagnostic is required for a violation of this rule.
>
> Although it is ill-formed no diagnostic required.


Thanks Shafik for pointing this out! I thought that this kind of scoping 
problems could happen only with FriendDecls.

Unfortunately, similar code constructs to the main example are there in live 
projects, the example is a reduced case from Google's Protobuf. Also, I 
understand that giving a diagnostic would require an additional round of 
analysis (in Sema) of the scope after that is completed and that could cause 
performance issues. This is a good target for a clang-tidy check though.

So, I think this leaves us with 3 possible directions:

1. Do not handle this case, i.e. leaving one node missing from the imported AST 
of `Container` (this means abandoning this patch). This would be OK since we 
are not required to handle ill-formed code. The problem with this option is 
that when we want to import `Container` again then we will find that 
structurally nonequivalent to the previously imported `Container`, because it 
has one less FriendDecl.
2. Do handle the case. This option is justified from the view of the task of 
the ASTImporter that is to copy ASTs (maybe even if they result from ill-formed 
code). But perhaps it is not the best idea to blindly copy all kind of 
malformed ASTs. E.g. ASTs built from debug info could be malformed and then it 
is better to have diagnostics from the Importer (next option).
3. Emit a diagnostic here from ASTImporter and return with an `Error` because 
the code is ill-formed and we can diagnose that. Still, we would be able to 
diagnose these kind of errors only in case of FriendDecls. But perhaps the task 
of reporting these kind of errors should be in clang-tidy rather.

This is a hard decision to make, but I vote for 2) then 3).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75922



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

[PATCH] D75851: [Analyzer][StreamChecker] Added evaluation of fseek.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

Probably we need to clarify when to make warnings. Originally I do not wanted a 
warning after for example failed read, because it is possible to retry the 
operation. But the standard says that after failed fread the file position is 
indeterminate, so a new read may get indeterminate values that is a case for 
warning. The character I/O functions may work not this way so after failed 
`fgetc` it may be possible to retry it and no warning is needed. Some 
operations such as `fclose` and `fseek` can be called in error (including EOF) 
state.

For this to work we need more kinds of error states, or save the type of the 
last operation in the state (and use it to determine if warning is needed). 
Probably the best is to have only a single "failed" state and determine what 
kind of error is possible based on the saved last operation type. A "feof" and 
"ferror" kind of error state is still needed because after a call to `feof` if 
it returned true the error type is fixed to EOF error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75851



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

[PATCH] D75406: Avoid including FileManager.h from SourceManager.h

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75406



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

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.
Herald added a reviewer: jdoerfert.

So if I understand the history here:

- the `IsOMPStructuredBlock` bit on `Stmt` was added to implement 
`getStructuredBlock()`
- after review, `getStructuredBlock()` doesn't use the bit
- the bit is used in the textual AST dump, and an AST matcher (which is in turn 
never used in-tree)

Can we have the bit back please :-)
We're currently trying to add a HasErrors bit to improve error-recovery and 
some nodes are completely full.
Using a bit on every Stmt to improve the text dump a little and provide a 
matcher (only relevant to OMP files) seems disproportionately expensive.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59214



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

Re: [PATCH] D65994: Extended FPOptions with new attributes

[Serge Pavlov via cfe-commits]

It is a matter of taste, but for me it looks strange to develop complex and
error-prone system to save 7 bits at expense of readability and
maintainability. My observations show that clang AST consumes much less
memory than llvm objects.

FPOption could be shared between user using something like shared_ptr, but
it means expenses of 64 bit for a pointer. Don't know if it makes sense...


Thanks,
--Serge


On Wed, Mar 11, 2020 at 6:30 PM Sam McCall via Phabricator <
revi...@reviews.llvm.org> wrote:

> sammccall added a comment.
>
> This patch increased the used size of BinaryOperator by 5 bits.
> Those bits were all padding, but now BinaryOperatorBitfields is completely
> full at 32 bits and we can't add any new bits to Stmt without increasing
> BinaryOperator by 8 bytes. (See D75443 
> and D54526  for the optimization this
> would revert).
>
> To squeeze in the new bit I'm planning to suggest squeezing getInt() to 7
> bits (it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not
> really familiar with this domain - if many of the 90 states are not
> possible it'd probably be useful to have some more bits back :-)
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D65994/new/
>
> https://reviews.llvm.org/D65994
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75786: [clang-tidy] Move fuchsia-restrict-system-includes to portability module for general use.

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

@PaulkaToast This patch appears to have caused a buildbot issue, please can you 
investigate/revert: 
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/63869


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75786



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

[PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

[Aaron Ballman via Phabricator via cfe-commits]

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

Aside from minor nits, this seems reasonable to me, LG!




Comment at: clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:16
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/SetVector.h"
 #include 

Please sort the includes.



Comment at: clang/lib/AST/ASTContext.cpp:1619
 void ASTContext::addedLocalImportDecl(ImportDecl *Import) {
-  assert(!Import->NextLocalImport && "Import declaration already in the 
chain");
+  assert(!Import->getNextLocalImport() && "Import declaration already in the 
chain");
   assert(!Import->isFromASTFile() && "Non-local import declaration");

80-col limit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75784



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

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

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45
+  enum KindTy {
+Opened, /// Stream is opened.
+Closed, /// Closed stream (an invalid stream pointer after it was closed).
+OpenFailed /// The last open operation has failed.
+  } State;
+
+  /// The error state of a stream.

xazax.hun wrote:
> Szelethus wrote:
> > balazske wrote:
> > > Szelethus wrote:
> > > > Hmm, now that I think of it, could we just merge these 2 enums? Also, I 
> > > > fear that indexers would accidentally assign the comment to the enum 
> > > > after the comma:
> > > > 
> > > > ```lang=cpp
> > > > Opened, /// Stream is opened.
> > > > Closed, /// Closed stream (an invalid stream pointer after it was 
> > > > closed).
> > > > OpenFailed /// The last open operation has failed.
> > > > ```
> > > > ` /// Stream is opened` might be assigned to `Closed`. How about this:
> > > > ```lang=cpp
> > > > /// Stream is opened.
> > > > Opened,
> > > > /// Closed stream (an invalid stream pointer after it was closed).
> > > > Closed,
> > > > /// The last open operation has failed.
> > > > OpenFailed
> > > > ```
> > > Probably these can be merged, it is used for a stream that is in an 
> > > indeterminate state after failed `freopen`, but practically it is handled 
> > > the same way as a closed stream. But this change would be done in another 
> > > revision.
> > I meant to merge the two enums (`StreamState` and `ErrorKindTy`) and the 
> > fields associated with them (`State` and `ErrorState`). We could however 
> > merge `Closed` and `OpenFailed`, granted that we put a `NoteTag` to 
> > `evalFreopen`. But I agree, that should be another revision's topic :)
> Since you mentioned that ErrorState is only applicable to Open streams, I am 
> also +1 on merging the enums. These two states are not orthogonal, no reason 
> for them to be separate.
Not orthogonal, but rather hiearchical. That is a reason for not merging. I am 
completely against it.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:43
+EofError,   /// EOF condition (`feof` is true).
+OtherError, /// Other (non-EOF) error (`ferror` is true).
+AnyError/// EofError or OtherError, used if exact error is unknown.

balazske wrote:
> Szelethus wrote:
> > Shouldn't we call this `FError` instead, if this is set precisely when 
> > `ferror()` is true? Despite the comments, I still managed to get myself 
> > confused with it :)
> Plan is to rename to `NoError`, `FEofError`, `FErrorError`, 
> `FEofOrFErrorError`.
If would suggest  `NoError`, `FEoF`, `FError`, `FEoFOrFError`. Too many `Error` 
suffixes reduce the readability.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:51
+
+  bool isNoError() const { return ErrorState == NoError; }
+  bool isEofError() const { return ErrorState == EofError; }

balazske wrote:
> Szelethus wrote:
> > If a stream's opening failed, its still an error, yet this getter would 
> > return true for it. I think this is yet another reason to just merge the 2 
> > enums :)
> But as the comment says, the error state is applicable only in the opened 
> state. If not opened, we may not call the "error" functions at all (assertion 
> can be added). Anyway it is possible to merge the enums, then the `isOpened` 
> will be a bit difficult (true if state is one of `OpenedNoError`, 
> `OpenedFErrorError`, `OpenedFEofError`, `OpenedFEofOrFErrorError`). Logically 
> it is better to see that the state is an aggregation of two states, an 
> "openedness" and an error state. Probably this does not matter much here 
> because it is unlikely that new states will appear that make the code more 
> complex.
I would not merge the two enums: this way they are hierarchical. When only 
checking the openness we do not need to check for 4 different states. I 
completely agree with @balazske here.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:54
+  bool isOtherError() const { return ErrorState == OtherError; }
+  bool isAnyError() const { return ErrorState == AnyError; }
+

Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > This is confusing -- I would expect a method called `isAnyError()` to 
> > > return true when `isNoError()` is false.
> > The error state kinds are not mutually exclusive. The "any error" means we 
> > know that there is some error but we do not know what the error is (it is 
> > not relevant because nothing was done that depends on it). If a function is 
> > called that needs to know if "ferror" or "feof" is the error, the state 
> > will be split to `FErrorError` and `FEofError`.
> How about we change the name of it to `isUnknownError`? My main issue is that 
> to me, the name `isAnyError()` answeres the question whether the stream is in 
> any form of erroneous 

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D75917#1916160 , @sameerds wrote:

> how this builtin fits in with the overall scheme of language-specific and 
> target-specific details of an atomic operation. For example, is this meant 
> only for OpenCL? Does it work with CUDA? Or HIP? What is the behaviour for 
> scope in C++?


Identical to the fence instruction. Which is assumed well thought through 
already, given it's an IR instruction.

As far as I can tell, fence composes sensibly with other IR then generates the 
right thing at the back end. So it looks fit for purpose, just not currently 
available from clang.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3707
+Value *Scope = EmitScalarExpr(E->getArg(1));
+auto ScopeModel = AtomicScopeModel::create(AtomicScopeModelKind::OpenCL);
+

sameerds wrote:
> The proposed builtin does not claim to be an OpenCL builtin, so it's probably 
> not correct to simply assume the OpenCL model. Should the model be chosen 
> based on the source language specified?
The only values for AtomicScopeModelKind are none and OpenCL.


Repository:
  rC Clang

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

https://reviews.llvm.org/D75917



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

[PATCH] D75983: [clang-format] Improved identification of C# nullables

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe created this revision.
jbcoe added a reviewer: krasimir.
jbcoe added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Allow '?' inside C# generics.

  

Do not mistake casts like (Type?) as conditional operators.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75983

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -650,6 +650,10 @@
 
   verifyFormat(R"(int?[] arr = new int?[10];)",
Style); // An array of a nullable type.
+
+  verifyFormat(R"(var x = (int?)y;)", Style); // Cast to a nullable type.
+
+  verifyFormat(R"(var x = new MyContainer();)", Style); // Generics.
 }
 
 TEST_F(FormatTestCSharp, CSharpArraySubscripts) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -131,7 +131,7 @@
   }
   if (CurrentToken->isOneOf(tok::r_paren, tok::r_square, tok::r_brace) ||
   (CurrentToken->isOneOf(tok::colon, tok::question) && InExprContext &&
-   Style.Language != FormatStyle::LK_Proto &&
+   !Style.isCSharp() && Style.Language != FormatStyle::LK_Proto &&
Style.Language != FormatStyle::LK_TextProto))
 return false;
   // If a && or || is found and interpreted as a binary operator, this set
@@ -1013,12 +1013,13 @@
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   if (Style.isCSharp()) {
-// `Type? name;` and `Type? name =` can only be nullable types.
+// `Type?)`, `Type?>`, `Type? name;`and `Type? name =` can only be
+// nullable types.
 // Line.MustBeDeclaration will be true for `Type? name;`.
-if (!Contexts.back().IsExpression &&
-(Line.MustBeDeclaration ||
- (Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
-  Tok->Next->Next->is(tok::equal {
+if ((!Contexts.back().IsExpression && Line.MustBeDeclaration) ||
+(Tok->Next && Tok->Next->isOneOf(tok::r_paren, tok::greater)) ||
+(Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
+ Tok->Next->Next->is(tok::equal))) {
   Tok->Type = TT_CSharpNullable;
   break;
 }
@@ -2961,10 +2962,12 @@
 if (Right.is(TT_CSharpNullable))
   return false;
 
-// Require space after ? in nullable types.
-if (Left.is(TT_CSharpNullable))
+// Require space after ? in nullable types except in generics and casts.
+if (Left.is(TT_CSharpNullable)) {
+  if (Right.isOneOf(TT_TemplateCloser, tok::r_paren))
+return false;
   return true;
-
+}
 // No space before or after '?.'.
 if (Left.is(TT_CSharpNullConditional) || 
Right.is(TT_CSharpNullConditional))
   return false;


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -650,6 +650,10 @@
 
   verifyFormat(R"(int?[] arr = new int?[10];)",
Style); // An array of a nullable type.
+
+  verifyFormat(R"(var x = (int?)y;)", Style); // Cast to a nullable type.
+
+  verifyFormat(R"(var x = new MyContainer();)", Style); // Generics.
 }
 
 TEST_F(FormatTestCSharp, CSharpArraySubscripts) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -131,7 +131,7 @@
   }
   if (CurrentToken->isOneOf(tok::r_paren, tok::r_square, tok::r_brace) ||
   (CurrentToken->isOneOf(tok::colon, tok::question) && InExprContext &&
-   Style.Language != FormatStyle::LK_Proto &&
+   !Style.isCSharp() && Style.Language != FormatStyle::LK_Proto &&
Style.Language != FormatStyle::LK_TextProto))
 return false;
   // If a && or || is found and interpreted as a binary operator, this set
@@ -1013,12 +1013,13 @@
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   if (Style.isCSharp()) {
-// `Type? name;` and `Type? name =` can only be nullable types.
+// `Type?)`, `Type?>`, `Type? name;`and `Type? name =` can only be
+// nullable types.
 // Line.MustBeDeclaration will be true for `Type? name;`.
-if (!Contexts.back().IsExpression &&
-(Line.MustBeDeclaration ||
- (Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
-  Tok->Next->Next->is(tok::equal {
+if ((!Contexts.back().IsExpressio

[PATCH] D75851: [Analyzer][StreamChecker] Added evaluation of fseek.

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

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:382-384
+  C.addTransition(StateNotFailed);
+  C.addTransition(StateFailedWithFError);
+  C.addTransition(StateFailedWithoutFError);

Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > This seems a bit excessive, we could merge the last two into `FSeekError`.
> > There are 3 cases:
> >  - `fseek` did not fail at all. Return value is zero. This is 
> > `StateNotFailed`.
> >  - `fseek` failed but none of the error flags is true afterwards. Return 
> > value is nonzero but `feof` and `ferror` are not true. This is 
> > `StateFailedWithoutFError`.
> >  - `fseek` failed and we have `feof` or `ferror` set (never both). Return 
> > value is nonzero and `feof` or `ferror` will be true. This is 
> > `StateFailedWithFError`. And an use of `AnyError`, otherwise we need here 2 
> > states, one for `feof` and one for `ferror`. But it is not important here 
> > if `feof` or `ferror` is actually true, so the special value `AnyError` is 
> > used and only one new state instead of two.
> Sure, but from the point of the analyzer the latter two are the same, isn't 
> it? Its never a good idea to use a stream on which `fseek` failed without 
> checking.
> 
> State splits are the most expensive things the analyzer can make, which is 
> why I'm cautious here.
Somehow, this should be done with just two new states. Maybe there should be an 
error state "Unknown" instead of `OtherError` (or `FeoFOrFError` what I 
suggested at the other patch) which can be any of the errors plus the `NoError` 
value. Later, when `ferror()` of `feof()` is checked we can do a second state 
split, but not earlier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75851



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

[PATCH] D75985: clangd doc: Show a test case for clangd with some commands

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru created this revision.
sylvestre.ledru added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay, ilya-biryukov.
Herald added a project: clang.
sylvestre.ledru updated this revision to Diff 249595.
sylvestre.ledru added a comment.

Add a link to the doc


clangd is designed for IDE usage but having a simple test case
explains how it works. 
It will help newcomers or potential users.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75985

Files:
  clang-tools-extra/docs/clangd/SimpleExample.rst
  clang-tools-extra/docs/clangd/index.rst

Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -8,6 +8,7 @@
Installation
Features
Configuration
+   SimpleExample
 
 What is clangd?
 ===
Index: clang-tools-extra/docs/clangd/SimpleExample.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clangd/SimpleExample.rst
@@ -0,0 +1,109 @@
+=
+Simple example clangd
+=
+
+clangd is not designed to be used in command line.
+However, we are providing this example to show how it works.
+
+The following file (ex: commands.json) represent a list of commands.
+
+.. code-block:: json
+
+   {
+ "jsonrpc": "2.0",
+ "id": 0,
+ "method": "initialize",
+ "params": {
+   "capabilities": {
+ "textDocument": {
+   "completion": {
+ "completionItem": {
+   "snippetSupport": true
+ }
+   }
+ }
+   },
+   "trace": "off"
+ }
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "method": "textDocument/didOpen",
+   "params": {
+   "textDocument": {
+   "uri": "test:///main.cpp",
+   "languageId": "cpp",
+   "version": 1,
+   "text": "int func_with_args(int a, int b);\nint main() {\nfunc_with\n}"
+   }
+   }
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "id": 1,
+   "method": "textDocument/completion",
+   "params": {
+   "textDocument": {
+   "uri": "test:///main.cpp"
+   },
+   "position": {
+   "line": 2,
+   "character": 7
+}
+}
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "id": 4,
+   "method": "shutdown"
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "method": "exit"
+   }
+
+
+To run all the commands at once:
+
+.. code-block:: shell
+
+   clangd -lit-test < commands.json
+
+This example will try to get the completion on line 2, column 7.
+Here is a subset of the returned information:
+
+.. code-block:: json
+
+[...]
+"items": [
+  {
+"detail": "int",
+"filterText": "func_with_args",
+"insertText": "func_with_args(${1:int a}, ${2:int b})",
+"insertTextFormat": 2,
+"kind": 3,
+"label": " func_with_args(int a, int b)",
+"score": 9.905722045898,
+"sortText": "3ee1func_with_args",
+"textEdit": {
+  "newText": "func_with_args(${1:int a}, ${2:int b})",
+  "range": {
+"end": {
+  "character": 7,
+  "line": 2
+},
+"start": {
+  "character": 0,
+  "line": 2
+}
+  }
+}
+  }
+[...]
+
+`newText` shows that clangd is able to find a suggestion for the completion and
+where to insert it.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75983: [clang-format] Improved identification of C# nullables

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:1016
   if (Style.isCSharp()) {
-// `Type? name;` and `Type? name =` can only be nullable types.
+// `Type?)`, `Type?>`, `Type? name;`and `Type? name =` can only be
+// nullable types.

nit: add a space before `and` in the comment:
```
// ... `Type? name;`and ...
```



Comment at: clang/lib/Format/TokenAnnotator.cpp:2967
+if (Left.is(TT_CSharpNullable)) {
+  if (Right.isOneOf(TT_TemplateCloser, tok::r_paren))
+return false;

nit: just replace the outer if body by:
`return !Right.isOneOf(TT_TemplateCloser, tok::r_paren);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75983



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45
+  enum KindTy {
+Opened, /// Stream is opened.
+Closed, /// Closed stream (an invalid stream pointer after it was closed).
+OpenFailed /// The last open operation has failed.
+  } State;
+
+  /// The error state of a stream.

baloghadamsoftware wrote:
> xazax.hun wrote:
> > Szelethus wrote:
> > > balazske wrote:
> > > > Szelethus wrote:
> > > > > Hmm, now that I think of it, could we just merge these 2 enums? Also, 
> > > > > I fear that indexers would accidentally assign the comment to the 
> > > > > enum after the comma:
> > > > > 
> > > > > ```lang=cpp
> > > > > Opened, /// Stream is opened.
> > > > > Closed, /// Closed stream (an invalid stream pointer after it was 
> > > > > closed).
> > > > > OpenFailed /// The last open operation has failed.
> > > > > ```
> > > > > ` /// Stream is opened` might be assigned to `Closed`. How about this:
> > > > > ```lang=cpp
> > > > > /// Stream is opened.
> > > > > Opened,
> > > > > /// Closed stream (an invalid stream pointer after it was closed).
> > > > > Closed,
> > > > > /// The last open operation has failed.
> > > > > OpenFailed
> > > > > ```
> > > > Probably these can be merged, it is used for a stream that is in an 
> > > > indeterminate state after failed `freopen`, but practically it is 
> > > > handled the same way as a closed stream. But this change would be done 
> > > > in another revision.
> > > I meant to merge the two enums (`StreamState` and `ErrorKindTy`) and the 
> > > fields associated with them (`State` and `ErrorState`). We could however 
> > > merge `Closed` and `OpenFailed`, granted that we put a `NoteTag` to 
> > > `evalFreopen`. But I agree, that should be another revision's topic :)
> > Since you mentioned that ErrorState is only applicable to Open streams, I 
> > am also +1 on merging the enums. These two states are not orthogonal, no 
> > reason for them to be separate.
> Not orthogonal, but rather hiearchical. That is a reason for not merging. I 
> am completely against it.
In a more expressive language each enum value could have parameters and we 
could have
```
Opened(ErrorKind),
Closed,
OpenFailed
```

While we do not have such an expressive language, we can simulate this using 
the current constructs such as a variant. The question is, does this worth the 
effort? At this point this is more like the matter of taste as long as it is 
properly documented. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Balázs Kéri via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
+StreamSym, StreamState::getOpenedWithOtherError());
+C.addTransition(StateEof);
+C.addTransition(StateOther);

baloghadamsoftware wrote:
> xazax.hun wrote:
> > As you probably know we are splitting the execution paths here. This will 
> > potentially result in doubling the analysis tome for a function for each 
> > `feof` call. Since state splits can be really bad for performance we need 
> > good justification for each of them.
> > So the questions are:
> > * Is it really important to differentiate between eof and other error or is 
> > it possible to just have an any error state?
> > * Do we find more bugs in real world code that justifies these state splits?
> I agree here. Do not introduce such forks without specific reason.
`feof` and `ferror` should return the same value if called multiple times (and 
no other file operations in between). If the exact error is not saved in the 
state, how can we ensure that the calls return the same value?



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:404-405
+
+void StreamChecker::evalFerror(const FnDescription *Desc, const CallEvent 
&Call,
+   CheckerContext &C) const {
+  ProgramStateRef State = C.getState();

baloghadamsoftware wrote:
> balazske wrote:
> > Szelethus wrote:
> > > This function is practically the same as `evalFeof`, can we merge them?
> > It is not the same code ("EOF" and "other error" are interchanged), but can 
> > be merged somehow.
> Maybe using a template with a functor parameter and pass a lambda in the two 
> instantiations? (See `DebugContainerModeling` and `DebugIteratorModeling`) 
> and also some example is `Iterator.cpp`.
I have already a template implementation, but probably it is not needed now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D75786: [clang-tidy] Move fuchsia-restrict-system-includes to portability module for general use.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D75786#1916637 , @RKSimon wrote:

> @PaulkaToast This patch appears to have caused a buildbot issue, please can 
> you investigate/revert: 
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/63869


I believe the issue is that we're looking for system headers that are not 
mocked as part of the test. Sorry about not catching that earlier in the 
review! @PaulkaToast, you should create an empty `stdio.h` (and others used by 
your test) in Inputs/fucscia-restrict-system-includes/system so that we're not 
finding the actual system includes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75786



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

[PATCH] D75985: clangd doc: Show a test case for clangd with some commands

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru updated this revision to Diff 249595.
sylvestre.ledru added a comment.

Add a link to the doc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75985

Files:
  clang-tools-extra/docs/clangd/SimpleExample.rst
  clang-tools-extra/docs/clangd/index.rst

Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -8,6 +8,7 @@
Installation
Features
Configuration
+   SimpleExample
 
 What is clangd?
 ===
Index: clang-tools-extra/docs/clangd/SimpleExample.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clangd/SimpleExample.rst
@@ -0,0 +1,109 @@
+=
+Simple example clangd
+=
+
+clangd is not designed to be used in command line.
+However, we are providing this example to show how it works.
+
+The following file (ex: commands.json) represent a list of commands.
+
+.. code-block:: json
+
+   {
+ "jsonrpc": "2.0",
+ "id": 0,
+ "method": "initialize",
+ "params": {
+   "capabilities": {
+ "textDocument": {
+   "completion": {
+ "completionItem": {
+   "snippetSupport": true
+ }
+   }
+ }
+   },
+   "trace": "off"
+ }
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "method": "textDocument/didOpen",
+   "params": {
+   "textDocument": {
+   "uri": "test:///main.cpp",
+   "languageId": "cpp",
+   "version": 1,
+   "text": "int func_with_args(int a, int b);\nint main() {\nfunc_with\n}"
+   }
+   }
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "id": 1,
+   "method": "textDocument/completion",
+   "params": {
+   "textDocument": {
+   "uri": "test:///main.cpp"
+   },
+   "position": {
+   "line": 2,
+   "character": 7
+}
+}
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "id": 4,
+   "method": "shutdown"
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "method": "exit"
+   }
+
+
+To run all the commands at once:
+
+.. code-block:: shell
+
+   clangd -lit-test < commands.json
+
+This example will try to get the completion on line 2, column 7.
+Here is a subset of the returned information:
+
+.. code-block:: json
+
+[...]
+"items": [
+  {
+"detail": "int",
+"filterText": "func_with_args",
+"insertText": "func_with_args(${1:int a}, ${2:int b})",
+"insertTextFormat": 2,
+"kind": 3,
+"label": " func_with_args(int a, int b)",
+"score": 9.905722045898,
+"sortText": "3ee1func_with_args",
+"textEdit": {
+  "newText": "func_with_args(${1:int a}, ${2:int b})",
+  "range": {
+"end": {
+  "character": 7,
+  "line": 2
+},
+"start": {
+  "character": 0,
+  "line": 2
+}
+  }
+}
+  }
+[...]
+
+`newText` shows that clangd is able to find a suggestion for the completion and
+where to insert it.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75984: [clang-format] No space in 'new()' and 'this[Type x]' in C#

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe created this revision.
jbcoe added a reviewer: krasimir.
jbcoe added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75984

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -627,6 +627,8 @@
   verifyFormat(R"(taskContext.Factory.Run(async () => doThing(args);)", Style);
   verifyFormat(R"(catch (TestException) when (innerFinallyExecuted))", Style);
   verifyFormat(R"(private float[,] Values;)", Style);
+  verifyFormat(R"(Result this[Index x] => Foo(x);)", Style);
+  verifyFormat(R"(class ItemFactory where T : new() {})", Style);
 
   Style.SpacesInSquareBrackets = true;
   verifyFormat(R"(private float[ , ] Values;)", Style);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2931,6 +2931,14 @@
 // interpolated strings. Interpolated strings are merged into a single 
token
 // so cannot have spaces inserted by this function.
 
+// No space between 'this' and '['
+if (Left.is(tok::kw_this) && Right.is(tok::l_square))
+  return false;
+
+// No space between 'new' and '('
+if (Left.is(tok::kw_new) && Right.is(tok::l_paren))
+  return false;
+
 // Space before { (including space within '{ {').
 if (Right.is(tok::l_brace))
   return true;


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -627,6 +627,8 @@
   verifyFormat(R"(taskContext.Factory.Run(async () => doThing(args);)", Style);
   verifyFormat(R"(catch (TestException) when (innerFinallyExecuted))", Style);
   verifyFormat(R"(private float[,] Values;)", Style);
+  verifyFormat(R"(Result this[Index x] => Foo(x);)", Style);
+  verifyFormat(R"(class ItemFactory where T : new() {})", Style);
 
   Style.SpacesInSquareBrackets = true;
   verifyFormat(R"(private float[ , ] Values;)", Style);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2931,6 +2931,14 @@
 // interpolated strings. Interpolated strings are merged into a single token
 // so cannot have spaces inserted by this function.
 
+// No space between 'this' and '['
+if (Left.is(tok::kw_this) && Right.is(tok::l_square))
+  return false;
+
+// No space between 'new' and '('
+if (Left.is(tok::kw_new) && Right.is(tok::l_paren))
+  return false;
+
 // Space before { (including space within '{ {').
 if (Right.is(tok::l_brace))
   return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 5c917bd - [clang-format] No space in `new()` and `this[Type x]` in C#

[Jonathan Coe via cfe-commits]

Author: Jonathan Coe
Date: 2020-03-11T12:53:53Z
New Revision: 5c917bd9a7de8fc45401da00cd27661b429887e9

URL: 
https://github.com/llvm/llvm-project/commit/5c917bd9a7de8fc45401da00cd27661b429887e9
DIFF: 
https://github.com/llvm/llvm-project/commit/5c917bd9a7de8fc45401da00cd27661b429887e9.diff

LOG: [clang-format] No space in `new()` and `this[Type x]` in C#

Reviewers: krasimir

Reviewed By: krasimir

Subscribers: cfe-commits, MyDeveloperDay

Tags: #clang-format, #clang

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

Added: 


Modified: 
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/FormatTestCSharp.cpp

Removed: 




diff  --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index db230583b6c9..cf481a1dcb62 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2930,6 +2930,14 @@ bool TokenAnnotator::spaceRequiredBefore(const 
AnnotatedLine &Line,
 // interpolated strings. Interpolated strings are merged into a single 
token
 // so cannot have spaces inserted by this function.
 
+// No space between 'this' and '['
+if (Left.is(tok::kw_this) && Right.is(tok::l_square))
+  return false;
+
+// No space between 'new' and '('
+if (Left.is(tok::kw_new) && Right.is(tok::l_paren))
+  return false;
+
 // Space before { (including space within '{ {').
 if (Right.is(tok::l_brace))
   return true;

diff  --git a/clang/unittests/Format/FormatTestCSharp.cpp 
b/clang/unittests/Format/FormatTestCSharp.cpp
index 7f819a61c70e..824eb51bd693 100644
--- a/clang/unittests/Format/FormatTestCSharp.cpp
+++ b/clang/unittests/Format/FormatTestCSharp.cpp
@@ -627,6 +627,8 @@ TEST_F(FormatTestCSharp, CSharpSpaces) {
   verifyFormat(R"(taskContext.Factory.Run(async () => doThing(args);)", Style);
   verifyFormat(R"(catch (TestException) when (innerFinallyExecuted))", Style);
   verifyFormat(R"(private float[,] Values;)", Style);
+  verifyFormat(R"(Result this[Index x] => Foo(x);)", Style);
+  verifyFormat(R"(class ItemFactory where T : new() {})", Style);
 
   Style.SpacesInSquareBrackets = true;
   verifyFormat(R"(private float[ , ] Values;)", Style);



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

Re: [PATCH] D65994: Extended FPOptions with new attributes

[Sam McCall via cfe-commits]

On Wed, Mar 11, 2020 at 12:42 PM Serge Pavlov  wrote:

> It is a matter of taste, but for me it looks strange to develop complex
> and error-prone system to save 7 bits at expense of readability and
> maintainability. My observations show that clang AST consumes much less
> memory than llvm objects.
>
I agree, and would be happy if we had a design without these scarce bits,
but AFAICS we don't. In this environment, 5 bits are quite a lot for FP
flexibility, and I think the complexity of reducing this is small and
isolated (rounding + exceptions together fit in 4 bits). But I ask about
the domain because maybe there's a simple but smaller model I can't really
infer this myself, both rounding and exceptions seem essentially unused at
present.
(Note that the cost here is not 7 bits per node but 63 - once the bits
available in Stmt are full the stmt subclass needs to gain a member, and
its alignment is 8 *bytes*)

Use cases vary, and many tools deal with large ASTs but don't use LLVM
codegen at all.

FPOption could be shared between user using something like shared_ptr, but
> it means expenses of 64 bit for a pointer. Don't know if it makes sense...
>
As you say, I don't think this saves anything, unless we can stop storing
the pointer in BinaryOperator.

Cheers, Sam


>
>
> Thanks,
> --Serge
>
>
> On Wed, Mar 11, 2020 at 6:30 PM Sam McCall via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> sammccall added a comment.
>>
>> This patch increased the used size of BinaryOperator by 5 bits.
>> Those bits were all padding, but now BinaryOperatorBitfields is
>> completely full at 32 bits and we can't add any new bits to Stmt without
>> increasing BinaryOperator by 8 bytes. (See D75443 <
>> https://reviews.llvm.org/D75443> and D54526 <
>> https://reviews.llvm.org/D54526> for the optimization this would revert).
>>
>> To squeeze in the new bit I'm planning to suggest squeezing getInt() to 7
>> bits (it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not
>> really familiar with this domain - if many of the 90 states are not
>> possible it'd probably be useful to have some more bits back :-)
>>
>>
>> Repository:
>>   rG LLVM Github Monorepo
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D65994/new/
>>
>> https://reviews.llvm.org/D65994
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 1fb9c29 - [clang-format] Improved identification of C# nullables

[Jonathan Coe via cfe-commits]

Author: Jonathan Coe
Date: 2020-03-11T12:58:52Z
New Revision: 1fb9c29833ab88c4a3d6fda997839754d998

URL: 
https://github.com/llvm/llvm-project/commit/1fb9c29833ab88c4a3d6fda997839754d998
DIFF: 
https://github.com/llvm/llvm-project/commit/1fb9c29833ab88c4a3d6fda997839754d998.diff

LOG: [clang-format] Improved identification of C# nullables

Summary:
Allow `?` inside C# generics.

Do not mistake casts like `(Type?)` as conditional operators.

Reviewers: krasimir

Subscribers: cfe-commits, MyDeveloperDay

Tags: #clang-format, #clang

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

Added: 


Modified: 
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/FormatTestCSharp.cpp

Removed: 




diff  --git a/clang/lib/Format/TokenAnnotator.cpp 
b/clang/lib/Format/TokenAnnotator.cpp
index cf481a1dcb62..d546a9f7c606 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -131,7 +131,7 @@ class AnnotatingParser {
   }
   if (CurrentToken->isOneOf(tok::r_paren, tok::r_square, tok::r_brace) ||
   (CurrentToken->isOneOf(tok::colon, tok::question) && InExprContext &&
-   Style.Language != FormatStyle::LK_Proto &&
+   !Style.isCSharp() && Style.Language != FormatStyle::LK_Proto &&
Style.Language != FormatStyle::LK_TextProto))
 return false;
   // If a && or || is found and interpreted as a binary operator, this set
@@ -1013,12 +1013,13 @@ class AnnotatingParser {
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   if (Style.isCSharp()) {
-// `Type? name;` and `Type? name =` can only be nullable types.
+// `Type?)`, `Type?>`, `Type? name;` and `Type? name =` can only be
+// nullable types.
 // Line.MustBeDeclaration will be true for `Type? name;`.
-if (!Contexts.back().IsExpression &&
-(Line.MustBeDeclaration ||
- (Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
-  Tok->Next->Next->is(tok::equal {
+if ((!Contexts.back().IsExpression && Line.MustBeDeclaration) ||
+(Tok->Next && Tok->Next->isOneOf(tok::r_paren, tok::greater)) ||
+(Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
+ Tok->Next->Next->is(tok::equal))) {
   Tok->Type = TT_CSharpNullable;
   break;
 }
@@ -2969,9 +2970,9 @@ bool TokenAnnotator::spaceRequiredBefore(const 
AnnotatedLine &Line,
 if (Right.is(TT_CSharpNullable))
   return false;
 
-// Require space after ? in nullable types.
+// Require space after ? in nullable types except in generics and casts.
 if (Left.is(TT_CSharpNullable))
-  return true;
+  return !Right.isOneOf(TT_TemplateCloser, tok::r_paren);
 
 // No space before or after '?.'.
 if (Left.is(TT_CSharpNullConditional) || 
Right.is(TT_CSharpNullConditional))

diff  --git a/clang/unittests/Format/FormatTestCSharp.cpp 
b/clang/unittests/Format/FormatTestCSharp.cpp
index 824eb51bd693..03ebe337e76c 100644
--- a/clang/unittests/Format/FormatTestCSharp.cpp
+++ b/clang/unittests/Format/FormatTestCSharp.cpp
@@ -652,6 +652,10 @@ public class A {
 
   verifyFormat(R"(int?[] arr = new int?[10];)",
Style); // An array of a nullable type.
+
+  verifyFormat(R"(var x = (int?)y;)", Style); // Cast to a nullable type.
+
+  verifyFormat(R"(var x = new MyContainer();)", Style); // Generics.
 }
 
 TEST_F(FormatTestCSharp, CSharpArraySubscripts) {



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

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

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:43
+EofError,   /// EOF condition (`feof` is true).
+OtherError, /// Other (non-EOF) error (`ferror` is true).
+AnyError/// EofError or OtherError, used if exact error is unknown.

baloghadamsoftware wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Shouldn't we call this `FError` instead, if this is set precisely when 
> > > `ferror()` is true? Despite the comments, I still managed to get myself 
> > > confused with it :)
> > Plan is to rename to `NoError`, `FEofError`, `FErrorError`, 
> > `FEofOrFErrorError`.
> If would suggest  `NoError`, `FEoF`, `FError`, `FEoFOrFError`. Too many 
> `Error` suffixes reduce the readability.
I suggest to omit the last error state in this patch and introduce it in the 
patch which uses it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D75045: [analyzer] Improved check of `fgetc` in StreamChecker.

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

baloghadamsoftware added inline comments.



Comment at: clang/test/Analysis/stream.c:188
+C = fgetc(fp);
+fread(0, 0, 0, fp); // expected-warning {{Should call}}
+fread(0, 0, 0, fp);

Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Are we sure that this is the error message we want to emit here? Sure, 
> > > not checking whether the stream is in an error state is a recipe for 
> > > disaster, but this seems to clash with
> > > > Should call 'feof' and 'ferror' after failed character read.
> > > as error here is not even checking the return value. Shouldn't we say
> > > > Should check the return value of `fgetc` before ...
> > > instead?
> > No, it is not enough to check the return value of `fgetc`, exactly this is 
> > the reason for the checker. "Should call 'feof' and 'ferror' after failed 
> > character read." is more exact: The read was suspected to fail because EOF 
> > was returned but should call feof (...) to verify it. A better message can 
> > be:
> > 
> > If 'fgetc' returns EOF call 'feof' and 'ferror' to check for error.
> The compliant solution in the rule description has the following example:
> 
> ```lang=c++
> 
> #include 
>  
> void func(void) {
>   int c;
>  
>   do {
> c = getchar();
>   } while (c != EOF);
>   if (feof(stdin)) {
> /* Handle end of file */
>   } else if (ferror(stdin)) {
> /* Handle file error */
>   } else {
> /* Received a character that resembles EOF; handle error */
>   }
> }
> ```
> because if the character resembles `EOF` that is an error too, and also
> 
> > Calling both functions on each iteration of a loop adds significant 
> > overhead, so a good strategy is to temporarily trust EOF and WEOF within 
> > the loop but verify them with feof() and ferror() following the loop.
> 
> Which makes me thing that the error here is not checking against `EOF`, first 
> and foremost.
> 
> The warning message
> 
> >If 'fgetc' returns EOF call 'feof' and 'ferror' to check for error.
> 
> sounds great, I would just prefer to see it //after// we **know** that the 
> stream returned `EOF`.
I agree. The checker should emit a warning if and only if the return value of 
`fgetc()` equals to `EOF`. Another checker should ensure that the return value 
of `fgetc()` is indeed compared to `EOF`. However: are we sure that there are 
no cases were no other error may happen than //EOF// thus we intentionally omit 
further checking? (Imagine a simple read from the keyboard in a command line 
utility.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75045



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

[PATCH] D75984: [clang-format] No space in `new()` and `this[Type x]` in C#

[Krasimir Georgiev via Phabricator via cfe-commits]

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

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75984



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

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

I'm working on a patch that will move FPOptions to Trailing storage, part of 
https://reviews.llvm.org/D72841 ; hope to have another revision uploaded today 
or tomorrow.  I got feedback that moving the field to BinaryOperator isn't 
adequate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443



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

[PATCH] D75983: [clang-format] Improved identification of C# nullables

[Jonathan B Coe via Phabricator via cfe-commits]

jbcoe updated this revision to Diff 249596.
jbcoe added a comment.

Fix nits from review.


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

https://reviews.llvm.org/D75983

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -652,6 +652,10 @@
 
   verifyFormat(R"(int?[] arr = new int?[10];)",
Style); // An array of a nullable type.
+
+  verifyFormat(R"(var x = (int?)y;)", Style); // Cast to a nullable type.
+
+  verifyFormat(R"(var x = new MyContainer();)", Style); // Generics.
 }
 
 TEST_F(FormatTestCSharp, CSharpArraySubscripts) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -131,7 +131,7 @@
   }
   if (CurrentToken->isOneOf(tok::r_paren, tok::r_square, tok::r_brace) ||
   (CurrentToken->isOneOf(tok::colon, tok::question) && InExprContext &&
-   Style.Language != FormatStyle::LK_Proto &&
+   !Style.isCSharp() && Style.Language != FormatStyle::LK_Proto &&
Style.Language != FormatStyle::LK_TextProto))
 return false;
   // If a && or || is found and interpreted as a binary operator, this set
@@ -1013,12 +1013,13 @@
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   if (Style.isCSharp()) {
-// `Type? name;` and `Type? name =` can only be nullable types.
+// `Type?)`, `Type?>`, `Type? name;` and `Type? name =` can only be
+// nullable types.
 // Line.MustBeDeclaration will be true for `Type? name;`.
-if (!Contexts.back().IsExpression &&
-(Line.MustBeDeclaration ||
- (Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
-  Tok->Next->Next->is(tok::equal {
+if ((!Contexts.back().IsExpression && Line.MustBeDeclaration) ||
+(Tok->Next && Tok->Next->isOneOf(tok::r_paren, tok::greater)) ||
+(Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
+ Tok->Next->Next->is(tok::equal))) {
   Tok->Type = TT_CSharpNullable;
   break;
 }
@@ -2969,9 +2970,9 @@
 if (Right.is(TT_CSharpNullable))
   return false;
 
-// Require space after ? in nullable types.
+// Require space after ? in nullable types except in generics and casts.
 if (Left.is(TT_CSharpNullable))
-  return true;
+  return !Right.isOneOf(TT_TemplateCloser, tok::r_paren);
 
 // No space before or after '?.'.
 if (Left.is(TT_CSharpNullConditional) || 
Right.is(TT_CSharpNullConditional))


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -652,6 +652,10 @@
 
   verifyFormat(R"(int?[] arr = new int?[10];)",
Style); // An array of a nullable type.
+
+  verifyFormat(R"(var x = (int?)y;)", Style); // Cast to a nullable type.
+
+  verifyFormat(R"(var x = new MyContainer();)", Style); // Generics.
 }
 
 TEST_F(FormatTestCSharp, CSharpArraySubscripts) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -131,7 +131,7 @@
   }
   if (CurrentToken->isOneOf(tok::r_paren, tok::r_square, tok::r_brace) ||
   (CurrentToken->isOneOf(tok::colon, tok::question) && InExprContext &&
-   Style.Language != FormatStyle::LK_Proto &&
+   !Style.isCSharp() && Style.Language != FormatStyle::LK_Proto &&
Style.Language != FormatStyle::LK_TextProto))
 return false;
   // If a && or || is found and interpreted as a binary operator, this set
@@ -1013,12 +1013,13 @@
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   if (Style.isCSharp()) {
-// `Type? name;` and `Type? name =` can only be nullable types.
+// `Type?)`, `Type?>`, `Type? name;` and `Type? name =` can only be
+// nullable types.
 // Line.MustBeDeclaration will be true for `Type? name;`.
-if (!Contexts.back().IsExpression &&
-(Line.MustBeDeclaration ||
- (Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
-  Tok->Next->Next->is(tok::equal {
+if ((!Contexts.back().IsExpression && Line.MustBeDeclaration) ||
+(Tok->Next && Tok->Next->isOneOf(tok::r_paren, tok::greater)) ||
+(Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
+ Tok->Next->Next->is(tok::equal))) {

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

As discussed offline... this undoes a desirable optimization from D54526 
.

A couple of possible ways to avoid this:

- FPOptions integer encoding can fit into 7 bits: combine integer behavior + 
exception behavior into 3 x 5 = 15 states that fits in 4 bits.
- IsOMPStructuredBlock bit is basically unused (just for AST dump and an unused 
ASTMatcher), that bit could be dropped

Former is probably the eaisest.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443



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

[PATCH] D75983: [clang-format] Improved identification of C# nullables

[Krasimir Georgiev via Phabricator via cfe-commits]

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

Thank you!


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

https://reviews.llvm.org/D75983



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

[PATCH] D75983: [clang-format] Improved identification of C# nullables

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1fb9c29833ab: [clang-format] Improved identification of C# 
nullables (authored by Jonathan Coe ).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75983

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -652,6 +652,10 @@
 
   verifyFormat(R"(int?[] arr = new int?[10];)",
Style); // An array of a nullable type.
+
+  verifyFormat(R"(var x = (int?)y;)", Style); // Cast to a nullable type.
+
+  verifyFormat(R"(var x = new MyContainer();)", Style); // Generics.
 }
 
 TEST_F(FormatTestCSharp, CSharpArraySubscripts) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -131,7 +131,7 @@
   }
   if (CurrentToken->isOneOf(tok::r_paren, tok::r_square, tok::r_brace) ||
   (CurrentToken->isOneOf(tok::colon, tok::question) && InExprContext &&
-   Style.Language != FormatStyle::LK_Proto &&
+   !Style.isCSharp() && Style.Language != FormatStyle::LK_Proto &&
Style.Language != FormatStyle::LK_TextProto))
 return false;
   // If a && or || is found and interpreted as a binary operator, this set
@@ -1013,12 +1013,13 @@
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   if (Style.isCSharp()) {
-// `Type? name;` and `Type? name =` can only be nullable types.
+// `Type?)`, `Type?>`, `Type? name;` and `Type? name =` can only be
+// nullable types.
 // Line.MustBeDeclaration will be true for `Type? name;`.
-if (!Contexts.back().IsExpression &&
-(Line.MustBeDeclaration ||
- (Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
-  Tok->Next->Next->is(tok::equal {
+if ((!Contexts.back().IsExpression && Line.MustBeDeclaration) ||
+(Tok->Next && Tok->Next->isOneOf(tok::r_paren, tok::greater)) ||
+(Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
+ Tok->Next->Next->is(tok::equal))) {
   Tok->Type = TT_CSharpNullable;
   break;
 }
@@ -2969,9 +2970,9 @@
 if (Right.is(TT_CSharpNullable))
   return false;
 
-// Require space after ? in nullable types.
+// Require space after ? in nullable types except in generics and casts.
 if (Left.is(TT_CSharpNullable))
-  return true;
+  return !Right.isOneOf(TT_TemplateCloser, tok::r_paren);
 
 // No space before or after '?.'.
 if (Left.is(TT_CSharpNullConditional) || 
Right.is(TT_CSharpNullConditional))


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -652,6 +652,10 @@
 
   verifyFormat(R"(int?[] arr = new int?[10];)",
Style); // An array of a nullable type.
+
+  verifyFormat(R"(var x = (int?)y;)", Style); // Cast to a nullable type.
+
+  verifyFormat(R"(var x = new MyContainer();)", Style); // Generics.
 }
 
 TEST_F(FormatTestCSharp, CSharpArraySubscripts) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -131,7 +131,7 @@
   }
   if (CurrentToken->isOneOf(tok::r_paren, tok::r_square, tok::r_brace) ||
   (CurrentToken->isOneOf(tok::colon, tok::question) && InExprContext &&
-   Style.Language != FormatStyle::LK_Proto &&
+   !Style.isCSharp() && Style.Language != FormatStyle::LK_Proto &&
Style.Language != FormatStyle::LK_TextProto))
 return false;
   // If a && or || is found and interpreted as a binary operator, this set
@@ -1013,12 +1013,13 @@
   Style.Language == FormatStyle::LK_JavaScript)
 break;
   if (Style.isCSharp()) {
-// `Type? name;` and `Type? name =` can only be nullable types.
+// `Type?)`, `Type?>`, `Type? name;` and `Type? name =` can only be
+// nullable types.
 // Line.MustBeDeclaration will be true for `Type? name;`.
-if (!Contexts.back().IsExpression &&
-(Line.MustBeDeclaration ||
- (Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
-  Tok->Next->Next->is(tok::equal {
+if ((!Contexts.back().IsExpression && Line.MustBeDeclaration) ||
+(Tok->Next && Tok->Next->isOneOf(tok:

[PATCH] D75984: [clang-format] No space in `new()` and `this[Type x]` in C#

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5c917bd9a7de: [clang-format] No space in `new()` and 
`this[Type x]` in C# (authored by Jonathan Coe ).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75984

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -627,6 +627,8 @@
   verifyFormat(R"(taskContext.Factory.Run(async () => doThing(args);)", Style);
   verifyFormat(R"(catch (TestException) when (innerFinallyExecuted))", Style);
   verifyFormat(R"(private float[,] Values;)", Style);
+  verifyFormat(R"(Result this[Index x] => Foo(x);)", Style);
+  verifyFormat(R"(class ItemFactory where T : new() {})", Style);
 
   Style.SpacesInSquareBrackets = true;
   verifyFormat(R"(private float[ , ] Values;)", Style);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2930,6 +2930,14 @@
 // interpolated strings. Interpolated strings are merged into a single 
token
 // so cannot have spaces inserted by this function.
 
+// No space between 'this' and '['
+if (Left.is(tok::kw_this) && Right.is(tok::l_square))
+  return false;
+
+// No space between 'new' and '('
+if (Left.is(tok::kw_new) && Right.is(tok::l_paren))
+  return false;
+
 // Space before { (including space within '{ {').
 if (Right.is(tok::l_brace))
   return true;


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -627,6 +627,8 @@
   verifyFormat(R"(taskContext.Factory.Run(async () => doThing(args);)", Style);
   verifyFormat(R"(catch (TestException) when (innerFinallyExecuted))", Style);
   verifyFormat(R"(private float[,] Values;)", Style);
+  verifyFormat(R"(Result this[Index x] => Foo(x);)", Style);
+  verifyFormat(R"(class ItemFactory where T : new() {})", Style);
 
   Style.SpacesInSquareBrackets = true;
   verifyFormat(R"(private float[ , ] Values;)", Style);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2930,6 +2930,14 @@
 // interpolated strings. Interpolated strings are merged into a single token
 // so cannot have spaces inserted by this function.
 
+// No space between 'this' and '['
+if (Left.is(tok::kw_this) && Right.is(tok::l_square))
+  return false;
+
+// No space between 'new' and '('
+if (Left.is(tok::kw_new) && Right.is(tok::l_paren))
+  return false;
+
 // Space before { (including space within '{ {').
 if (Right.is(tok::l_brace))
   return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 249606.
hokein added a comment.

reduce FPOption from 8 bits to 7 bits, based on the offline discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443

Files:
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/LangOptions.h

Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -197,7 +197,7 @@
   /// Possible rounding modes.
   enum FPRoundingModeKind {
 /// Rounding to nearest, corresponds to "round.tonearest".
-FPR_ToNearest,
+FPR_ToNearest = 0,
 /// Rounding toward -Inf, corresponds to "round.downward".
 FPR_Downward,
 /// Rounding toward +Inf, corresponds to "round.upward".
@@ -211,7 +211,7 @@
   /// Possible floating point exception behavior.
   enum FPExceptionModeKind {
 /// Assume that floating-point exceptions are masked.
-FPE_Ignore,
+FPE_Ignore = 0,
 /// Transformations do not cause new exceptions but may hide some.
 FPE_MayTrap,
 /// Strictly preserve the floating-point exception semantics.
@@ -358,23 +358,20 @@
 public:
   FPOptions() : fp_contract(LangOptions::FPC_Off),
 fenv_access(LangOptions::FEA_Off),
-rounding(LangOptions::FPR_ToNearest),
-exceptions(LangOptions::FPE_Ignore)
+packed(0)
 {}
 
   // Used for serializing.
   explicit FPOptions(unsigned I)
   : fp_contract(static_cast(I & 3)),
 fenv_access(static_cast((I >> 2) & 1)),
-rounding(static_cast((I >> 3) & 7)),
-exceptions(static_cast((I >> 6) & 3))
+packed(I >> 3)
 {}
 
   explicit FPOptions(const LangOptions &LangOpts)
   : fp_contract(LangOpts.getDefaultFPContractMode()),
 fenv_access(LangOptions::FEA_Off),
-rounding(LangOptions::FPR_ToNearest),
-exceptions(LangOptions::FPE_Ignore)
+packed(0)
 {}
   // FIXME: Use getDefaultFEnvAccessMode() when available.
 
@@ -407,19 +404,24 @@
   void setDisallowFEnvAccess() { fenv_access = LangOptions::FEA_Off; }
 
   LangOptions::FPRoundingModeKind getRoundingMode() const {
+unsigned rounding = packed / MaxExceptionValue;
 return static_cast(rounding);
   }
 
   void setRoundingMode(LangOptions::FPRoundingModeKind RM) {
-rounding = RM;
+packed = RM * MaxExceptionValue + getExceptionMode();
   }
 
   LangOptions::FPExceptionModeKind getExceptionMode() const {
+static_assert(LangOptions::FPExceptionModeKind::FPE_Strict <
+  MaxExceptionValue,
+  "Max exception value must less than 3");
+unsigned exceptions = packed % MaxExceptionValue;
 return static_cast(exceptions);
   }
 
   void setExceptionMode(LangOptions::FPExceptionModeKind EM) {
-exceptions = EM;
+packed = getRoundingMode() * MaxExceptionValue + EM;
   }
 
   bool isFPConstrained() const {
@@ -430,8 +432,7 @@
 
   /// Used to serialize this.
   unsigned getInt() const {
-return fp_contract | (fenv_access << 2) | (rounding << 3)
-| (exceptions << 6);
+return fp_contract | (fenv_access << 2) | (packed << 3);
   }
 
 private:
@@ -440,8 +441,10 @@
   /// of these fields.
   unsigned fp_contract : 2;
   unsigned fenv_access : 1;
-  unsigned rounding : 3;
-  unsigned exceptions : 2;
+  // A packed field for encoding rounding and exceptions.
+  // packed = MaxExceptionBits * rounding + excpetions.
+  constexpr static unsigned MaxExceptionValue = 3;
+  unsigned packed: 4;
 };
 
 /// Describes the kind of translation unit being processed.
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -531,7 +531,7 @@
 
 /// This is only meaningful for operations on floating point
 /// types and 0 otherwise.
-unsigned FPFeatures : 8;
+unsigned FPFeatures : 7;
 
 SourceLocation OpLoc;
   };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 249607.
hokein added a comment.

update a missing CXXOperatorCallExprBitfields, and fix typos.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443

Files:
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/LangOptions.h

Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -197,7 +197,7 @@
   /// Possible rounding modes.
   enum FPRoundingModeKind {
 /// Rounding to nearest, corresponds to "round.tonearest".
-FPR_ToNearest,
+FPR_ToNearest = 0,
 /// Rounding toward -Inf, corresponds to "round.downward".
 FPR_Downward,
 /// Rounding toward +Inf, corresponds to "round.upward".
@@ -211,7 +211,7 @@
   /// Possible floating point exception behavior.
   enum FPExceptionModeKind {
 /// Assume that floating-point exceptions are masked.
-FPE_Ignore,
+FPE_Ignore = 0,
 /// Transformations do not cause new exceptions but may hide some.
 FPE_MayTrap,
 /// Strictly preserve the floating-point exception semantics.
@@ -358,23 +358,20 @@
 public:
   FPOptions() : fp_contract(LangOptions::FPC_Off),
 fenv_access(LangOptions::FEA_Off),
-rounding(LangOptions::FPR_ToNearest),
-exceptions(LangOptions::FPE_Ignore)
+packed(0)
 {}
 
   // Used for serializing.
   explicit FPOptions(unsigned I)
   : fp_contract(static_cast(I & 3)),
 fenv_access(static_cast((I >> 2) & 1)),
-rounding(static_cast((I >> 3) & 7)),
-exceptions(static_cast((I >> 6) & 3))
+packed(I >> 3)
 {}
 
   explicit FPOptions(const LangOptions &LangOpts)
   : fp_contract(LangOpts.getDefaultFPContractMode()),
 fenv_access(LangOptions::FEA_Off),
-rounding(LangOptions::FPR_ToNearest),
-exceptions(LangOptions::FPE_Ignore)
+packed(0)
 {}
   // FIXME: Use getDefaultFEnvAccessMode() when available.
 
@@ -407,19 +404,24 @@
   void setDisallowFEnvAccess() { fenv_access = LangOptions::FEA_Off; }
 
   LangOptions::FPRoundingModeKind getRoundingMode() const {
+unsigned rounding = packed / MaxExceptionValue;
 return static_cast(rounding);
   }
 
   void setRoundingMode(LangOptions::FPRoundingModeKind RM) {
-rounding = RM;
+packed = RM * MaxExceptionValue + getExceptionMode();
   }
 
   LangOptions::FPExceptionModeKind getExceptionMode() const {
+static_assert(LangOptions::FPExceptionModeKind::FPE_Strict <
+  MaxExceptionValue,
+  "Max exception value must less than 3");
+unsigned exceptions = packed % MaxExceptionValue;
 return static_cast(exceptions);
   }
 
   void setExceptionMode(LangOptions::FPExceptionModeKind EM) {
-exceptions = EM;
+packed = getRoundingMode() * MaxExceptionValue + EM;
   }
 
   bool isFPConstrained() const {
@@ -430,8 +432,7 @@
 
   /// Used to serialize this.
   unsigned getInt() const {
-return fp_contract | (fenv_access << 2) | (rounding << 3)
-| (exceptions << 6);
+return fp_contract | (fenv_access << 2) | (packed << 3);
   }
 
 private:
@@ -440,8 +441,10 @@
   /// of these fields.
   unsigned fp_contract : 2;
   unsigned fenv_access : 1;
-  unsigned rounding : 3;
-  unsigned exceptions : 2;
+  // A packed field for encoding rounding and exceptions.
+  // packed = MaxExceptionValue * rounding + exceptions.
+  constexpr static unsigned MaxExceptionValue = 3;
+  unsigned packed: 4;
 };
 
 /// Describes the kind of translation unit being processed.
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -531,7 +531,7 @@
 
 /// This is only meaningful for operations on floating point
 /// types and 0 otherwise.
-unsigned FPFeatures : 8;
+unsigned FPFeatures : 7;
 
 SourceLocation OpLoc;
   };
@@ -614,7 +614,7 @@
 unsigned OperatorKind : 6;
 
 // Only meaningful for floating point types.
-unsigned FPFeatures : 8;
+unsigned FPFeatures : 7;
   };
 
   class CXXRewrittenBinaryOperatorBitfields {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

What is better? Have this (or similar) change that adds a feature that is used 
in a later change and is only testable in that later change, or have a bigger 
change that contains real use of the added features? (This means: Add `fseek` 
to this change or not.) The error handling is not normally testable if there is 
no function that generates error state, `fseek` could be one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D75443#1916761 , @mibintc wrote:

> I'm working on a patch that will move FPOptions to Trailing storage, part of 
> https://reviews.llvm.org/D72841 ; hope to have another revision uploaded 
> today or tomorrow.  I got feedback that moving the field to BinaryOperator 
> isn't adequate.


Great! Moving FPOptions to trailing storage (which I guess allows it to only be 
present for floating point ops?) is much better than making it a member, and 
frees up a lot of bits.

To unblock adding the extra "error" bit that we need (which really does apply 
to all Exprs), we could do any of:

1. wait for D72841  to land, including the 
trailingstorage
2. move the trailing-storage parts out of that patch, and do it first as a 
smaller separate change
3. find some other way to save the bit for now (e.g. by encoding rounding + 
exceptions together), and undo that once saving 1 bit doesn't matter

Any objections if we do 3 and take care of rolling it back later?




Comment at: clang/include/clang/Basic/LangOptions.h:200
 /// Rounding to nearest, corresponds to "round.tonearest".
-FPR_ToNearest,
+FPR_ToNearest = 0,
 /// Rounding toward -Inf, corresponds to "round.downward".

this is implied and unneccesary



Comment at: clang/include/clang/Basic/LangOptions.h:361
 fenv_access(LangOptions::FEA_Off),
-rounding(LangOptions::FPR_ToNearest),
-exceptions(LangOptions::FPE_Ignore)
+packed(0)
 {}

Making use of the encoding of `packed` in lots of places is fragile and hard to 
maintain.

I'd suggest the private primitives:
`setRoundingAndExceptionMode(FPRoundingModeKind, FPExceptionModeKind)`
`getRoundingAndExceptionMode() -> pair`
and implementing everything in terms of them.




Comment at: clang/include/clang/Basic/LangOptions.h:445
+  // A packed field for encoding rounding and exceptions.
+  // packed = MaxExceptionValue * rounding + exceptions.
+  constexpr static unsigned MaxExceptionValue = 3;

FIXME: unpack this once saving one bit isn't critical here.



Comment at: clang/include/clang/Basic/LangOptions.h:447
+  constexpr static unsigned MaxExceptionValue = 3;
+  unsigned packed: 4;
 };

rounding_and_exceptions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443



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

[clang] edbf2fd - [analyzer] Fix a strange compile error on a certain Clang-7.0.0

[Artem Dergachev via cfe-commits]

Author: Artem Dergachev
Date: 2020-03-11T16:54:34+03:00
New Revision: edbf2fde14a2b50e64ea20a011b2a3242c75b4d9

URL: 
https://github.com/llvm/llvm-project/commit/edbf2fde14a2b50e64ea20a011b2a3242c75b4d9
DIFF: 
https://github.com/llvm/llvm-project/commit/edbf2fde14a2b50e64ea20a011b2a3242c75b4d9.diff

LOG: [analyzer] Fix a strange compile error on a certain Clang-7.0.0

error: default initialization of an object of const type
   'const clang::QualType' without a user-provided
   default constructor

  Irrelevant; // A placeholder, whenever we do not care about the type.
  ^
{}

Added: 


Modified: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

Removed: 




diff  --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 608015781c49..d52b3f371af9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -500,7 +500,7 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   // or long long, so three summary variants would be enough).
   // Of course, function variants are also useful for C++ overloads.
   const QualType
-  Irrelevant; // A placeholder, whenever we do not care about the type.
+  Irrelevant{}; // A placeholder, whenever we do not care about the type.
   const QualType IntTy = ACtx.IntTy;
   const QualType LongTy = ACtx.LongTy;
   const QualType LongLongTy = ACtx.LongLongTy;



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Balázs Kéri via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45
+  enum KindTy {
+Opened, /// Stream is opened.
+Closed, /// Closed stream (an invalid stream pointer after it was closed).
+OpenFailed /// The last open operation has failed.
+  } State;
+
+  /// The error state of a stream.

xazax.hun wrote:
> baloghadamsoftware wrote:
> > xazax.hun wrote:
> > > Szelethus wrote:
> > > > balazske wrote:
> > > > > Szelethus wrote:
> > > > > > Hmm, now that I think of it, could we just merge these 2 enums? 
> > > > > > Also, I fear that indexers would accidentally assign the comment to 
> > > > > > the enum after the comma:
> > > > > > 
> > > > > > ```lang=cpp
> > > > > > Opened, /// Stream is opened.
> > > > > > Closed, /// Closed stream (an invalid stream pointer after it 
> > > > > > was closed).
> > > > > > OpenFailed /// The last open operation has failed.
> > > > > > ```
> > > > > > ` /// Stream is opened` might be assigned to `Closed`. How about 
> > > > > > this:
> > > > > > ```lang=cpp
> > > > > > /// Stream is opened.
> > > > > > Opened,
> > > > > > /// Closed stream (an invalid stream pointer after it was 
> > > > > > closed).
> > > > > > Closed,
> > > > > > /// The last open operation has failed.
> > > > > > OpenFailed
> > > > > > ```
> > > > > Probably these can be merged, it is used for a stream that is in an 
> > > > > indeterminate state after failed `freopen`, but practically it is 
> > > > > handled the same way as a closed stream. But this change would be 
> > > > > done in another revision.
> > > > I meant to merge the two enums (`StreamState` and `ErrorKindTy`) and 
> > > > the fields associated with them (`State` and `ErrorState`). We could 
> > > > however merge `Closed` and `OpenFailed`, granted that we put a 
> > > > `NoteTag` to `evalFreopen`. But I agree, that should be another 
> > > > revision's topic :)
> > > Since you mentioned that ErrorState is only applicable to Open streams, I 
> > > am also +1 on merging the enums. These two states are not orthogonal, no 
> > > reason for them to be separate.
> > Not orthogonal, but rather hiearchical. That is a reason for not merging. I 
> > am completely against it.
> In a more expressive language each enum value could have parameters and we 
> could have
> ```
> Opened(ErrorKind),
> Closed,
> OpenFailed
> ```
> 
> While we do not have such an expressive language, we can simulate this using 
> the current constructs such as a variant. The question is, does this worth 
> the effort? At this point this is more like the matter of taste as long as it 
> is properly documented. 
Have a `bool isOpened` and an error kind is better?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 249613.
hokein marked 4 inline comments as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443

Files:
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/LangOptions.h

Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -356,26 +356,24 @@
 /// Floating point control options
 class FPOptions {
 public:
-  FPOptions() : fp_contract(LangOptions::FPC_Off),
-fenv_access(LangOptions::FEA_Off),
-rounding(LangOptions::FPR_ToNearest),
-exceptions(LangOptions::FPE_Ignore)
-{}
+  FPOptions()
+  : fp_contract(LangOptions::FPC_Off), fenv_access(LangOptions::FEA_Off) {
+setRoundingAndExceptionMode(LangOptions::FPR_ToNearest,
+LangOptions::FPE_Ignore);
+  }
 
   // Used for serializing.
   explicit FPOptions(unsigned I)
   : fp_contract(static_cast(I & 3)),
 fenv_access(static_cast((I >> 2) & 1)),
-rounding(static_cast((I >> 3) & 7)),
-exceptions(static_cast((I >> 6) & 3))
-{}
+rounding_and_exceptions(I >> 3) {}
 
   explicit FPOptions(const LangOptions &LangOpts)
   : fp_contract(LangOpts.getDefaultFPContractMode()),
-fenv_access(LangOptions::FEA_Off),
-rounding(LangOptions::FPR_ToNearest),
-exceptions(LangOptions::FPE_Ignore)
-{}
+fenv_access(LangOptions::FEA_Off) {
+setRoundingAndExceptionMode(LangOptions::FPR_ToNearest,
+LangOptions::FPE_Ignore);
+  }
   // FIXME: Use getDefaultFEnvAccessMode() when available.
 
   bool allowFPContractWithinStatement() const {
@@ -407,19 +405,19 @@
   void setDisallowFEnvAccess() { fenv_access = LangOptions::FEA_Off; }
 
   LangOptions::FPRoundingModeKind getRoundingMode() const {
-return static_cast(rounding);
+ return getRoundingAndExceptionMode().first;
   }
 
   void setRoundingMode(LangOptions::FPRoundingModeKind RM) {
-rounding = RM;
+setRoundingAndExceptionMode(RM, getExceptionMode());
   }
 
   LangOptions::FPExceptionModeKind getExceptionMode() const {
-return static_cast(exceptions);
+return getRoundingAndExceptionMode().second;
   }
 
   void setExceptionMode(LangOptions::FPExceptionModeKind EM) {
-exceptions = EM;
+setRoundingAndExceptionMode(getRoundingMode(), EM);
   }
 
   bool isFPConstrained() const {
@@ -430,18 +428,37 @@
 
   /// Used to serialize this.
   unsigned getInt() const {
-return fp_contract | (fenv_access << 2) | (rounding << 3)
-| (exceptions << 6);
+return fp_contract | (fenv_access << 2) | (rounding_and_exceptions << 3);
   }
 
 private:
+  void setRoundingAndExceptionMode(LangOptions::FPRoundingModeKind RM,
+   LangOptions::FPExceptionModeKind EM) {
+static_assert(LangOptions::FPExceptionModeKind::FPE_Strict <
+  MaxExceptionValue,
+  "Max exception value must be less than 3");
+rounding_and_exceptions = RM * MaxExceptionValue + EM;
+  }
+
+  std::pair
+  getRoundingAndExceptionMode() const {
+unsigned exceptions = rounding_and_exceptions % MaxExceptionValue;
+unsigned rounding = rounding_and_exceptions / MaxExceptionValue;
+return {static_cast(rounding),
+static_cast(exceptions)};
+  }
+
+
   /// Adjust BinaryOperatorBitfields::FPFeatures and
   /// CXXOperatorCallExprBitfields::FPFeatures to match the total bit-field size
   /// of these fields.
   unsigned fp_contract : 2;
   unsigned fenv_access : 1;
-  unsigned rounding : 3;
-  unsigned exceptions : 2;
+  // A packed field for encoding rounding and exceptions.
+  // FIXME: unpack this once saving one bit isn't critical here.
+  // rounding_and_exceptions = MaxExceptionValue * rounding + exceptions.
+  constexpr static unsigned MaxExceptionValue = 3;
+  unsigned rounding_and_exceptions: 4;
 };
 
 /// Describes the kind of translation unit being processed.
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -531,7 +531,7 @@
 
 /// This is only meaningful for operations on floating point
 /// types and 0 otherwise.
-unsigned FPFeatures : 8;
+unsigned FPFeatures : 7;
 
 SourceLocation OpLoc;
   };
@@ -614,7 +614,7 @@
 unsigned OperatorKind : 6;
 
 // Only meaningful for floating point types.
-unsigned FPFeatures : 8;
+unsigned FPFeatures : 7;
   };
 
   class CXXRewrittenBinaryOperatorBitfields {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D75443#1916829 , @sammccall wrote:

> In D75443#1916761 , @mibintc wrote:
>
> > I'm working on a patch that will move FPOptions to Trailing storage, part 
> > of https://reviews.llvm.org/D72841 ; hope to have another revision uploaded 
> > today or tomorrow.  I got feedback that moving the field to BinaryOperator 
> > isn't adequate.
>
>
> Great! Moving FPOptions to trailing storage (which I guess allows it to only 
> be present for floating point ops?) is much better than making it a member, 
> and frees up a lot of bits.
>
> To unblock adding the extra "error" bit that we need (which really does apply 
> to all Exprs), we could do any of:
>
> 1. wait for D72841  to land, including the 
> trailingstorage
> 2. move the trailing-storage parts out of that patch, and do it first as a 
> smaller separate change
> 3. find some other way to save the bit for now (e.g. by encoding rounding + 
> exceptions together), and undo that once saving 1 bit doesn't matter
>
>   Any objections if we do 3 and take care of rolling it back later?


I think waiting for 1 would probably take a long time since that is a huge 
patch. I'd prefer 3 to unblock the "error" bit stuff, but I'm also happy with 
2. what's your thought @mibintc?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

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

baloghadamsoftware added a comment.

In D75682#1916809 , @balazske wrote:

> What is better? Have this (or similar) change that adds a feature that is 
> used in a later change and is only testable in that later change, or have a 
> bigger change that contains real use of the added features? (This means: Add 
> `fseek` to this change or not.) The error handling is not normally testable 
> if there is no function that generates error state, `fseek` could be one.


I think neither: add the feature (the last enum value and its handling in the 
functions) in the `fseek()` patch were it is used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[David Tenty via Phabricator via cfe-commits]

daltenty added a comment.

On AIX and PPC LE Linux after this change we are seeing invalid accesses when 
the backend asserts/fatal_errors. Looks like the driver and CC1 now share some 
global TimerGroup state that points to Timers which got created by CC1 but 
didn't get cleanup after the assert. Reported as: 
https://bugs.llvm.org/show_bug.cgi?id=45164

Example failure:

  ./bin/clang ./llvm-project/clang/test/CodeGenObjC/illegal-UTF8.m -S -target 
powerpc-ibm-aix
  ...
  fatal error: error in backend: Unhandled linkage when mapping linkage to 
StorageClass.
  Stack dump:
  ...
  ***
  AddressSanitizer:DEADLYSIGNAL
  =
  ==37262==ERROR: AddressSanitizer: SEGV on unknown address 0x10715768 (pc 
0x3fffb7cdb2c4 bp 0x3e1fb764cdb0 sp 0x3fffc460 T0)
  ==37262==The signal is caused by a UNKNOWN memory access.
  #0 0x3fffb7cdb2c0 in std::basic_string, 
std::allocator >::basic_string(std::string const&) 
(/lib64/libstdc++.so.6+0x11b2c0)
  #1 0x16ca75c0 in PrintRecord 
/home/daltenty/llvm/dev/llvm-project/llvm/include/llvm/Support/Timer.h:180:21
  #2 0x16ca75c0 in construct &, std::basic_string &> 
/opt/rh/devtoolset-7/root/usr/lib/gcc/ppc64le-redhat-linux/7/../../../../include/c++/7/ext/new_allocator.h:136:23
  #3 0x16ca75c0 in construct &, std::basic_string &> 
/opt/rh/devtoolset-7/root/usr/lib/gcc/ppc64le-redhat-linux/7/../../../../include/c++/7/bits/alloc_traits.h:475:8
  #4 0x16ca75c0 in void std::vector 
>::_M_realloc_insert(__gnu_cxx::__normal_iterator > >, llvm::TimeRecord&, 
std::string&, std::string&) 
/opt/rh/devtoolset-7/root/usr/lib/gcc/ppc64le-redhat-linux/7/../../../../include/c++/7/bits/vector.tcc:415:4
  #5 0x16ca2220 in emplace_back 
&, std::basic_string &> 
/opt/rh/devtoolset-7/root/usr/lib/gcc/ppc64le-redhat-linux/7/../../../../include/c++/7/bits/vector.tcc:105:4
  #6 0x16ca2220 in llvm::TimerGroup::prepareToPrintList(bool) 
/home/daltenty/llvm/dev/llvm-project/llvm/lib/Support/Timer.cpp:359:19
  #7 0x16ca2498 in llvm::TimerGroup::print(llvm::raw_ostream&, bool) 
/home/daltenty/llvm/dev/llvm-project/llvm/lib/Support/Timer.cpp:373:5
  #8 0x16ca2a4c in llvm::TimerGroup::printAll(llvm::raw_ostream&) 
/home/daltenty/llvm/dev/llvm-project/llvm/lib/Support/Timer.cpp:391:9
  #9 0x107f1940 in main 
/home/daltenty/llvm/dev/llvm-project/clang/tools/driver/driver.cpp:535:3
  #10 0x3fffb79b497c in generic_start_main.isra.0 (/lib64/libc.so.6+0x2497c)
  
  AddressSanitizer can not provide additional info.
  SUMMARY: AddressSanitizer: SEGV (/lib64/libstdc++.so.6+0x11b2c0) in 
std::basic_string, std::allocator 
>::basic_string(std::string const&)
  ==37262==ABORTING




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth updated this revision to Diff 249616.
tnorth added a comment.

Rebased on master (6d5603e2 
). Some 
refactoring was indeed needed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

Files:
  clang/docs/ClangFormat.rst
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14912,6 +14912,61 @@
   auto StyleTd = getStyle("file", "x.td", "llvm", "", &FS);
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
+
+  // Test 9: explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file:/e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style8);
+  ASSERT_EQ(*Style8, getGoogleStyle());
+
+  // Test 10: relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style9 = getStyle("file:../../e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style9);
+  ASSERT_EQ(*Style9, getGoogleStyle());
+
+  // Test 11: missing explicit format file
+  auto Style10 = getStyle("file:/e/missing.clang-format",
+  "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style10);
+  llvm::consumeError(Style10.takeError());
+
+  // Test 12: format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  auto Style11 = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style11);
+  ASSERT_EQ(*Style11, getGoogleStyle());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2640,6 +2640,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -2685,6 +2687,29 @@
   return GuessedLanguage;
 }
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile,
+   llvm::vfs::FileSystem *FS,
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();
+  *IsSuitable = true;
+
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  if (std::error_code EC = Text.getError())
+return make_string_error(EC.message());
+  std::error_code ParserErrorCode =
+  parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParserErrorCode == ParseError::Unsuitable) {
+*IsSuitable = false;
+return Style;
+  } else if (ParserErrorCode != ParseError::Success) {
+return make_string_error("Error reading " + ConfigFile + ": " +
+ ParserErrorCode.message());
+  }
+  return Style;
+}
+
 const char *DefaultFormatStyle = "file";
 
 const char *DefaultFallbackStyle = "LLVM";
@@ -2709,6 +2734,21 @@
 return Style;
   }
 
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/p

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

well, it would probably be quickest to separate the trailing storage into a 
separate patch.  i'll try to get that submitted today.  shall i submit that as 
a phabricator review or is there a different, preferred way to do that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443



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

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D75443#1916909 , @mibintc wrote:

> well, it would probably be quickest to separate the trailing storage into a 
> separate patch.  i'll try to get that submitted today.  shall i submit that 
> as a phabricator review or is there a different, preferred way to do that?


(Sorry our comments keep racing)

If you don't mind doing that, it'd be great - phabricator is the best way.
I'm happy to do my best reviewing, but haven't made a similar change myself, so 
getting rjmccall's eyes on it would definitely get you a better check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443



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

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: sepavloff.
sammccall added a comment.

this looks reasonable to me, would like to give @mibintc and also @sepavloff a 
chance to chime in.




Comment at: clang/include/clang/Basic/LangOptions.h:437
+   LangOptions::FPExceptionModeKind EM) {
+static_assert(LangOptions::FPExceptionModeKind::FPE_Strict <
+  MaxExceptionValue,

This assertion seems weird, and it doesn't test much - new enum values are 
typically added at the end.



Comment at: clang/include/clang/Basic/LangOptions.h:460
+  // rounding_and_exceptions = MaxExceptionValue * rounding + exceptions.
+  constexpr static unsigned MaxExceptionValue = 3;
+  unsigned rounding_and_exceptions: 4;

nit: NumExceptionModes ?
(It's not the max value, it's max + 1)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443



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

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D75443#1916909 , @mibintc wrote:

> well, it would probably be quickest to separate the trailing storage into a 
> separate patch.  i'll try to get that submitted today.  shall i submit that 
> as a phabricator review or is there a different, preferred way to do that?


that sounds good to me, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

And why not add first the "unknown error" state and later the feof and ferror 
error states? Or add every error state but not the `feof` and `ferror` 
functions? Every way results in an intermediate state of the checker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D69825#1916865 , @daltenty wrote:

> On AIX and PPC LE Linux after this change we are seeing invalid accesses when 
> the backend asserts/fatal_errors. Looks like the driver and CC1 now share 
> some global TimerGroup state that points to Timers which got created by CC1 
> but didn't get cleanup after the assert. Reported as: 
> https://bugs.llvm.org/show_bug.cgi?id=45164


From the bug report, it looks like we are discovering more problems associated 
with this patch that affect the 10.0 release. I've marked the bug as blocking 
release-10.0.0 for triage purposes for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

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

Szelethus added a comment.

Also, thank you guys for chipping in!




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
+StreamSym, StreamState::getOpenedWithOtherError());
+C.addTransition(StateEof);
+C.addTransition(StateOther);

balazske wrote:
> baloghadamsoftware wrote:
> > xazax.hun wrote:
> > > As you probably know we are splitting the execution paths here. This will 
> > > potentially result in doubling the analysis tome for a function for each 
> > > `feof` call. Since state splits can be really bad for performance we need 
> > > good justification for each of them.
> > > So the questions are:
> > > * Is it really important to differentiate between eof and other error or 
> > > is it possible to just have an any error state?
> > > * Do we find more bugs in real world code that justifies these state 
> > > splits?
> > I agree here. Do not introduce such forks without specific reason.
> `feof` and `ferror` should return the same value if called multiple times 
> (and no other file operations in between). If the exact error is not saved in 
> the state, how can we ensure that the calls return the same value?
This is a very interesting question indeed, never crossed my mind before. I 
think it would be better to move `AnyError` to the next revision and discuss it 
there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

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

Szelethus added a comment.

I see that we're a bit stuck on naming, and that is largely my fault. 
Apologies, let us focus on high level stuff for a bit.

In D75682#1916435 , @balazske wrote:

> I do not understand totally this remove-of-AnyError thing. If handling the 
> `AnyError` will be removed the code remains still not testable.




In D75682#1915809 , @Szelethus wrote:

> Remove the code that adds and handles `AnyError`. Merge the two enums in the 
> `StreamState`, and make `ensureSteamOpened` emit a warning if the stream is 
> either `feof()` or `ferror()`. Add `NoteTags` saying something along the 
> lines of:
>
>   if (feof(F)) { // note: Assuming the condition is true
>  // note: Stream 'F' has the EOF flag set
> fgets(F); // warning: Illegal stream operation on a stream that has EOF 
> set
>   }
>  
>


If we added warnings to this patch, that would be testable.

In D75682#1916809 , @balazske wrote:

> What is better? Have this (or similar) change that adds a feature that is 
> used in a later change and is only testable in that later change, or have a 
> bigger change that contains real use of the added features? (This means: Add 
> `fseek` to this change or not.) The error handling is not normally testable 
> if there is no function that generates error state, `fseek` could be one.


Definitely the latter, and all you need to do is check whether the stream is in 
an error state in `ensureStreamOpened`, no need for fseek just yet.

In D75682#1916867 , 
@baloghadamsoftware wrote:

> I think neither: add the feature (the last enum value and its handling in the 
> functions) in the `fseek()` patch were it is used.


Yup.




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45
+  enum KindTy {
+Opened, /// Stream is opened.
+Closed, /// Closed stream (an invalid stream pointer after it was closed).
+OpenFailed /// The last open operation has failed.
+  } State;
+
+  /// The error state of a stream.

balazske wrote:
> xazax.hun wrote:
> > baloghadamsoftware wrote:
> > > xazax.hun wrote:
> > > > Szelethus wrote:
> > > > > balazske wrote:
> > > > > > Szelethus wrote:
> > > > > > > Hmm, now that I think of it, could we just merge these 2 enums? 
> > > > > > > Also, I fear that indexers would accidentally assign the comment 
> > > > > > > to the enum after the comma:
> > > > > > > 
> > > > > > > ```lang=cpp
> > > > > > > Opened, /// Stream is opened.
> > > > > > > Closed, /// Closed stream (an invalid stream pointer after it 
> > > > > > > was closed).
> > > > > > > OpenFailed /// The last open operation has failed.
> > > > > > > ```
> > > > > > > ` /// Stream is opened` might be assigned to `Closed`. How about 
> > > > > > > this:
> > > > > > > ```lang=cpp
> > > > > > > /// Stream is opened.
> > > > > > > Opened,
> > > > > > > /// Closed stream (an invalid stream pointer after it was 
> > > > > > > closed).
> > > > > > > Closed,
> > > > > > > /// The last open operation has failed.
> > > > > > > OpenFailed
> > > > > > > ```
> > > > > > Probably these can be merged, it is used for a stream that is in an 
> > > > > > indeterminate state after failed `freopen`, but practically it is 
> > > > > > handled the same way as a closed stream. But this change would be 
> > > > > > done in another revision.
> > > > > I meant to merge the two enums (`StreamState` and `ErrorKindTy`) and 
> > > > > the fields associated with them (`State` and `ErrorState`). We could 
> > > > > however merge `Closed` and `OpenFailed`, granted that we put a 
> > > > > `NoteTag` to `evalFreopen`. But I agree, that should be another 
> > > > > revision's topic :)
> > > > Since you mentioned that ErrorState is only applicable to Open streams, 
> > > > I am also +1 on merging the enums. These two states are not orthogonal, 
> > > > no reason for them to be separate.
> > > Not orthogonal, but rather hiearchical. That is a reason for not merging. 
> > > I am completely against it.
> > In a more expressive language each enum value could have parameters and we 
> > could have
> > ```
> > Opened(ErrorKind),
> > Closed,
> > OpenFailed
> > ```
> > 
> > While we do not have such an expressive language, we can simulate this 
> > using the current constructs such as a variant. The question is, does this 
> > worth the effort? At this point this is more like the matter of taste as 
> > long as it is properly documented. 
> Have a `bool isOpened` and an error kind is better?
That sounds wonderful, @balazske! :)



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:44
+OtherError, /// Other (non-EOF) error (`ferror` is true).
+AnyError/// EofError or OtherError, used if exact error is unknown.
+  } ErrorState = NoError;
--

Re: [PATCH] D65994: Extended FPOptions with new attributes

[Serge Pavlov via cfe-commits]

>
>In this environment, 5 bits are quite a lot for FP flexibility, and I
> think the complexity of reducing this is small and isolated (rounding +
> exceptions together fit in 4 bits)


Rounding (5 standard variants) and exception (3 variants) already do not
fit 4 bits. And there is also precision, denormals treatment, exception
mask, they are usually represented in hardware register and
make up a floating point environment. Instead of putting all these bits to
different places it would be better to collect them in one place, it could
make operations simpler and faster.

  both rounding and exceptions seem essentially unused at present.


Implementation of advanced floating point support is in progress, they will
be used more and more.

  (Note that the cost here is not 7 bits per node but 63 - once the bits
> available in Stmt are full the stmt subclass needs to gain a member, and
> its alignment is 8 *bytes*)


As objects are allocated aligned, if size in not multiple of the alignment,
some part of memory is spent uselessly anyway.

Use cases vary, and many tools deal with large ASTs but don't use LLVM
> codegen at all.


That is true, but developer tools are usually less restricted in computing
resources. Besides hard economy in memory often means some loss of
performance.

FPOption could be shared between user using something like shared_ptr, but
>> it means expenses of 64 bit for a pointer. Don't know if it makes sense...
>> As you say, I don't think this saves anything, unless we can stop storing
>> the pointer in BinaryOperator.
>
>
We could keep pointer to shared properties, thus saving memory. Moreover
such facility could enable some techniques using this shared property as a
marker. For example, all floating point operations obtained from the same
region where `pragma STDC FENV_ACCESS` is in act could point to the same
FPOption object, thus we could distinguish between different regions. It
would save memory consumption without sacrifice of maintainability and
speed.

Thanks,
--Serge


On Wed, Mar 11, 2020 at 8:01 PM Sam McCall  wrote:

> On Wed, Mar 11, 2020 at 12:42 PM Serge Pavlov  wrote:
>
>> It is a matter of taste, but for me it looks strange to develop complex
>> and error-prone system to save 7 bits at expense of readability and
>> maintainability. My observations show that clang AST consumes much less
>> memory than llvm objects.
>>
> I agree, and would be happy if we had a design without these scarce bits,
> but AFAICS we don't. In this environment, 5 bits are quite a lot for FP
> flexibility, and I think the complexity of reducing this is small and
> isolated (rounding + exceptions together fit in 4 bits). But I ask about
> the domain because maybe there's a simple but smaller model I can't really
> infer this myself, both rounding and exceptions seem essentially unused at
> present.
> (Note that the cost here is not 7 bits per node but 63 - once the bits
> available in Stmt are full the stmt subclass needs to gain a member, and
> its alignment is 8 *bytes*)
>
> Use cases vary, and many tools deal with large ASTs but don't use LLVM
> codegen at all.
>
> FPOption could be shared between user using something like shared_ptr, but
>> it means expenses of 64 bit for a pointer. Don't know if it makes sense...
>>
> As you say, I don't think this saves anything, unless we can stop storing
> the pointer in BinaryOperator.
>
> Cheers, Sam
>
>
>>
>>
>> Thanks,
>> --Serge
>>
>>
>> On Wed, Mar 11, 2020 at 6:30 PM Sam McCall via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> sammccall added a comment.
>>>
>>> This patch increased the used size of BinaryOperator by 5 bits.
>>> Those bits were all padding, but now BinaryOperatorBitfields is
>>> completely full at 32 bits and we can't add any new bits to Stmt without
>>> increasing BinaryOperator by 8 bytes. (See D75443 <
>>> https://reviews.llvm.org/D75443> and D54526 <
>>> https://reviews.llvm.org/D54526> for the optimization this would
>>> revert).
>>>
>>> To squeeze in the new bit I'm planning to suggest squeezing getInt() to
>>> 7 bits (it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not
>>> really familiar with this domain - if many of the 90 states are not
>>> possible it'd probably be useful to have some more bits back :-)
>>>
>>>
>>> Repository:
>>>   rG LLVM Github Monorepo
>>>
>>> CHANGES SINCE LAST ACTION
>>>   https://reviews.llvm.org/D65994/new/
>>>
>>> https://reviews.llvm.org/D65994
>>>
>>>
>>>
>>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] d83ade4 - [clangd] Improve the "max limit" error message in rename, NFC.

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2020-03-11T16:13:46+01:00
New Revision: d83ade4506089329217918439042b68093e8d6e4

URL: 
https://github.com/llvm/llvm-project/commit/d83ade4506089329217918439042b68093e8d6e4
DIFF: 
https://github.com/llvm/llvm-project/commit/d83ade4506089329217918439042b68093e8d6e4.diff

LOG: [clangd] Improve the "max limit" error message in rename, NFC.

previously, we emited "exceeds the max limit 49" which was weird, now we
emit "exceeds the max limit 50".

Added: 


Modified: 
clang-tools-extra/clangd/refactor/Rename.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/refactor/Rename.cpp 
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 54112e09f0f9..91620920c6ac 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -308,7 +308,7 @@ findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
   // Absolute file path => rename occurrences in that file.
   llvm::StringMap> AffectedFiles;
   bool HasMore = Index.refs(RQuest, [&](const Ref &R) {
-if (AffectedFiles.size() > MaxLimitFiles)
+if (AffectedFiles.size() >= MaxLimitFiles)
   return;
 if ((R.Kind & RefKind::Spelled) == RefKind::Unknown)
   return;
@@ -318,7 +318,7 @@ findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
 }
   });
 
-  if (AffectedFiles.size() > MaxLimitFiles)
+  if (AffectedFiles.size() >= MaxLimitFiles)
 return llvm::make_error(
 llvm::formatv("The number of affected files exceeds the max limit {0}",
   MaxLimitFiles),
@@ -521,7 +521,7 @@ llvm::Expected rename(const RenameInputs 
&RInputs) {
 auto OtherFilesEdits = renameOutsideFile(
 RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
 Opts.LimitFiles == 0 ? std::numeric_limits::max()
- : Opts.LimitFiles - 1,
+ : Opts.LimitFiles,
 GetFileContent);
 if (!OtherFilesEdits)
   return OtherFilesEdits.takeError();



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

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

[Sameer Sahasrabuddhe via Phabricator via cfe-commits]

sameerds added a comment.

In D75917#1916664 , @JonChesterfield 
wrote:

> In D75917#1916160 , @sameerds wrote:
>
> > how this builtin fits in with the overall scheme of language-specific and 
> > target-specific details of an atomic operation. For example, is this meant 
> > only for OpenCL? Does it work with CUDA? Or HIP? What is the behaviour for 
> > scope in C++?
>
>
> Identical to the fence instruction. Which is assumed well thought through 
> already, given it's an IR instruction.
>
> As far as I can tell, fence composes sensibly with other IR then generates 
> the right thing at the back end. So it looks fit for purpose, just not 
> currently available from clang.


Well, there is a problem: The LangRef says that scopes are target-defined. This 
change says that scopes are defined by the high-level language and further 
assumes that OpenCL scopes make sense in all languages. Besides conflicting 
with the LangRef, this not seem to work with C++, which has no scopes and nor 
with CUDA or HIP, whose scopes are not represented in any AtomicScopeModel.


Repository:
  rC Clang

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

https://reviews.llvm.org/D75917



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

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

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45
+  enum KindTy {
+Opened, /// Stream is opened.
+Closed, /// Closed stream (an invalid stream pointer after it was closed).
+OpenFailed /// The last open operation has failed.
+  } State;
+
+  /// The error state of a stream.

Szelethus wrote:
> balazske wrote:
> > xazax.hun wrote:
> > > baloghadamsoftware wrote:
> > > > xazax.hun wrote:
> > > > > Szelethus wrote:
> > > > > > balazske wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > Hmm, now that I think of it, could we just merge these 2 enums? 
> > > > > > > > Also, I fear that indexers would accidentally assign the 
> > > > > > > > comment to the enum after the comma:
> > > > > > > > 
> > > > > > > > ```lang=cpp
> > > > > > > > Opened, /// Stream is opened.
> > > > > > > > Closed, /// Closed stream (an invalid stream pointer after 
> > > > > > > > it was closed).
> > > > > > > > OpenFailed /// The last open operation has failed.
> > > > > > > > ```
> > > > > > > > ` /// Stream is opened` might be assigned to `Closed`. How 
> > > > > > > > about this:
> > > > > > > > ```lang=cpp
> > > > > > > > /// Stream is opened.
> > > > > > > > Opened,
> > > > > > > > /// Closed stream (an invalid stream pointer after it was 
> > > > > > > > closed).
> > > > > > > > Closed,
> > > > > > > > /// The last open operation has failed.
> > > > > > > > OpenFailed
> > > > > > > > ```
> > > > > > > Probably these can be merged, it is used for a stream that is in 
> > > > > > > an indeterminate state after failed `freopen`, but practically it 
> > > > > > > is handled the same way as a closed stream. But this change would 
> > > > > > > be done in another revision.
> > > > > > I meant to merge the two enums (`StreamState` and `ErrorKindTy`) 
> > > > > > and the fields associated with them (`State` and `ErrorState`). We 
> > > > > > could however merge `Closed` and `OpenFailed`, granted that we put 
> > > > > > a `NoteTag` to `evalFreopen`. But I agree, that should be another 
> > > > > > revision's topic :)
> > > > > Since you mentioned that ErrorState is only applicable to Open 
> > > > > streams, I am also +1 on merging the enums. These two states are not 
> > > > > orthogonal, no reason for them to be separate.
> > > > Not orthogonal, but rather hiearchical. That is a reason for not 
> > > > merging. I am completely against it.
> > > In a more expressive language each enum value could have parameters and 
> > > we could have
> > > ```
> > > Opened(ErrorKind),
> > > Closed,
> > > OpenFailed
> > > ```
> > > 
> > > While we do not have such an expressive language, we can simulate this 
> > > using the current constructs such as a variant. The question is, does 
> > > this worth the effort? At this point this is more like the matter of 
> > > taste as long as it is properly documented. 
> > Have a `bool isOpened` and an error kind is better?
> That sounds wonderful, @balazske! :)
Yes, we do not need the `OpenFailed` state either. A stream is either opened or 
not. This is the //state// of the stream. If there is any error, there are the 
//error codes// for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D74973: [analyzer] StdLibraryFunctionsChecker refactor w/ inheritance

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

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:151
+
+  using ValueConstraintPtr = std::shared_ptr;
+  /// The complete list of constraints that defines a single branch.

martong wrote:
> Szelethus wrote:
> > martong wrote:
> > > martong wrote:
> > > > Szelethus wrote:
> > > > > gamesh411 wrote:
> > > > > > martong wrote:
> > > > > > > Note here, we need a copyable, polymorphic and default 
> > > > > > > initializable type (vector needs that). A raw pointer were good, 
> > > > > > > however, we cannot default initialize that. unique_ptr makes the 
> > > > > > > Summary class non-copyable, therefore not an option.
> > > > > > > Releasing the copyablitly requirement would render the 
> > > > > > > initialization of the Summary map infeasible.
> > > > > > > Perhaps we could come up with a [[ 
> > > > > > > https://www.youtube.com/watch?v=bIhUE5uUFOA | type erasure 
> > > > > > > technique without inheritance ]] once we consider the shared_ptr 
> > > > > > > as restriction, but for now that seems to be overkill.
> > > > > > std::variant (with std::monostate for the default constructibility) 
> > > > > > would also be an option  (if c++17 were supported). But this is not 
> > > > > > really an issue, i agree with that.
> > > > > Ugh, we've historically been very hostile towards virtual functions. 
> > > > > We don't mind them that much when they don't have to run a lot, like 
> > > > > during bug report construction, but as a core part of the analysis, 
> > > > > I'm not sure what the current stance is on it.
> > > > > 
> > > > > I'm not inherently (haha) against it, and I'm fine with leaving this 
> > > > > as-is for the time being, though I'd prefer if you placed a `TODO` to 
> > > > > revisit this issue.
> > > > > std::variant (with std::monostate for the default constructibility) 
> > > > > would also be an option (if c++17 were supported). But this is not 
> > > > > really an issue, i agree with that.
> > > > 
> > > > Variant would be useful if we knew the set of classes prior and we 
> > > > wanted to add operations gradually. Class hierarchies (or run-time 
> > > > concepts [Sean Parent]) are very useful if we know the set of 
> > > > operations prior and we want to add classes gradually, and we have this 
> > > > case here.
> > > > Ugh, we've historically been very hostile towards virtual functions. We 
> > > > don't mind them that much when they don't have to run a lot, like 
> > > > during bug report construction, but as a core part of the analysis, I'm 
> > > > not sure what the current stance is on it.
> > > 
> > > I did not find any evidence for this statement. Consider as a counter 
> > > example the ExternalASTSource interface in Clang, which is filled with 
> > > virtual functions and is part of the C/C++ lookup mechanism, which is 
> > > quite on the hot path of C/C++ parsing I think. Did not find any 
> > > prohibition in LLVM coding guidelines neither. I do believe virtual 
> > > functions have their use cases exactly where (runtime) polimorphism is 
> > > needed, such as in this patch.
> > > 
> > I consider myself proven wrong here then.
> Thanks for the review and for considering other alternatives! And please 
> accept my apologies, maybe I was pushing too hard on inheritance.
We should definitely decorate this with a `TODO: Can we change this to not use 
a shared_ptr?`. Worst case scenario, there it will stay for eternity :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74973



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

[PATCH] D75995: [clangd] Decouple preamble and mainfile builds, NFCI

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, jfb, arphaman, jkorous, 
MaskRay, javed.absar, ilya-biryukov.
Herald added a project: clang.

First step to enable deferred preamble builds. Not intending to land it
alone, will have follow-ups that will implement full deferred build
functionality and will land after all of them are ready.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75995

Files:
  clang-tools-extra/clangd/Compiler.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/Threading.h

Index: clang-tools-extra/clangd/Threading.h
===
--- clang-tools-extra/clangd/Threading.h
+++ clang-tools-extra/clangd/Threading.h
@@ -107,7 +107,7 @@
   /// Destructor waits for all pending tasks to finish.
   ~AsyncTaskRunner();
 
-  void wait() const { (void)wait(Deadline::infinity()); }
+  void wait() const { (void)wait(timeoutSeconds(20)); }
   LLVM_NODISCARD bool wait(Deadline D) const;
   // The name is used for tracing and debugging (e.g. to name a spawned thread).
   void runAsync(const llvm::Twine &Name, llvm::unique_function Action);
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -49,28 +49,41 @@
 #include "GlobalCompilationDatabase.h"
 #include "Logger.h"
 #include "ParsedAST.h"
+#include "Path.h"
 #include "Preamble.h"
+#include "Threading.h"
 #include "Trace.h"
 #include "index/CanonicalIncludes.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Threading.h"
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
 using std::chrono::steady_clock;
 
 namespace {
-class ASTWorker;
+class MainFileWorker;
 } // namespace
 
 static clang::clangd::Key kFileBeingProcessed;
@@ -87,7 +100,7 @@
 /// Workers borrow ASTs when active, and return them when done.
 class TUScheduler::ASTCache {
 public:
-  using Key = const ASTWorker *;
+  using Key = const MainFileWorker *;
 
   ASTCache(unsigned MaxRetainedASTs) : MaxRetainedASTs(MaxRetainedASTs) {}
 
@@ -149,363 +162,389 @@
 };
 
 namespace {
-class ASTWorkerHandle;
-
-/// Owns one instance of the AST, schedules updates and reads of it.
-/// Also responsible for building and providing access to the preamble.
-/// Each ASTWorker processes the async requests sent to it on a separate
-/// dedicated thread.
-/// The ASTWorker that manages the AST is shared by both the processing thread
-/// and the TUScheduler. The TUScheduler should discard an ASTWorker when
-/// remove() is called, but its thread may be busy and we don't want to block.
-/// So the workers are accessed via an ASTWorkerHandle. Destroying the handle
-/// signals the worker to exit its run loop and gives up shared ownership of the
-/// worker.
-class ASTWorker {
-  friend class ASTWorkerHandle;
-  ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB,
-TUScheduler::ASTCache &LRUCache, Semaphore &Barrier, bool RunSync,
-DebouncePolicy UpdateDebounce, bool StorePreamblesInMemory,
-ParsingCallbacks &Callbacks);
 
+/// Common base class for MainFile and Preamble workers. Holds information
+/// needed for both and supports triggering a stop.
+class StoppableTUWorker {
 public:
-  /// Create a new ASTWorker and return a handle to it.
-  /// The processing thread is spawned using \p Tasks. However, when \p Tasks
-  /// is null, all requests will be processed on the calling thread
-  /// synchronously instead. \p Barrier is acquired when processing each
-  /// request, it is used to limit the number of actively running threads.
-  static ASTWorkerHandle
-  create(PathRef FileName, const GlobalCompilationDatabase &CDB,
- TUScheduler::ASTCache &IdleASTs, AsyncTaskRunner *Tasks,
- Semaphore &Barrier, DebouncePolicy UpdateDebounce,
- bool StorePreamblesInMemory, ParsingCallbacks &Callbacks);
-  ~ASTWorker();
-
-  void update(ParseInputs Inputs, WantDiagnostics);
-  void
-  runWithAST(llvm::StringRef Name,
- llvm::unique_function)> Action,
- TUScheduler::ASTActionInvalidation);
-  bool blockUntilIdle(Deadline Timeout) const;
-
-  std::shared_ptr getPossiblyStalePreamble() const;
-
-  /// Obtain a preamble reflecting all updates so

[PATCH] D75851: [Analyzer][StreamChecker] Added evaluation of fseek.

[Balázs Kéri via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:382-384
+  C.addTransition(StateNotFailed);
+  C.addTransition(StateFailedWithFError);
+  C.addTransition(StateFailedWithoutFError);

baloghadamsoftware wrote:
> Szelethus wrote:
> > balazske wrote:
> > > Szelethus wrote:
> > > > This seems a bit excessive, we could merge the last two into 
> > > > `FSeekError`.
> > > There are 3 cases:
> > >  - `fseek` did not fail at all. Return value is zero. This is 
> > > `StateNotFailed`.
> > >  - `fseek` failed but none of the error flags is true afterwards. Return 
> > > value is nonzero but `feof` and `ferror` are not true. This is 
> > > `StateFailedWithoutFError`.
> > >  - `fseek` failed and we have `feof` or `ferror` set (never both). Return 
> > > value is nonzero and `feof` or `ferror` will be true. This is 
> > > `StateFailedWithFError`. And an use of `AnyError`, otherwise we need here 
> > > 2 states, one for `feof` and one for `ferror`. But it is not important 
> > > here if `feof` or `ferror` is actually true, so the special value 
> > > `AnyError` is used and only one new state instead of two.
> > Sure, but from the point of the analyzer the latter two are the same, isn't 
> > it? Its never a good idea to use a stream on which `fseek` failed without 
> > checking.
> > 
> > State splits are the most expensive things the analyzer can make, which is 
> > why I'm cautious here.
> Somehow, this should be done with just two new states. Maybe there should be 
> an error state "Unknown" instead of `OtherError` (or `FeoFOrFError` what I 
> suggested at the other patch) which can be any of the errors plus the 
> `NoError` value. Later, when `ferror()` of `feof()` is checked we can do a 
> second state split, but not earlier.
Yes it is possible to use two new states only. For example by having a new 
`FSeekError` that can "manifest" in feof, ferror or no error. Better is to have 
only one "last-operation-failed-with-any-error" state and determine later if 
needed what errors are possible (based on the last operation). For `fseek` it 
is possible to get feof, ferror, or none of these (but still failed `fseek`). 
For reading operations the error can be feof or ferror. After write only ferror 
is possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75851



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

[PATCH] D75443: [AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.h:461
+  constexpr static unsigned MaxExceptionValue = 3;
+  unsigned rounding_and_exceptions: 4;
 };

There are 5 rounding directions defined by IEEE-754, which we should eventually 
support. Some targets supports additional rounding modes, which we might want 
to keep in FP environment. Existing implementation uses 3 exception behavior 
modes and we could want to extend them. For example it makes sense to introduce 
a special mode where Inexact exception is ignored but others are maintained. 
Four bits are too small space for FP environment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75443



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

[PATCH] D73898: [analyzer] StdLibraryFunctionsChecker: Add argument constraints

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

Szelethus added a comment.

Just littering some more inlines, don't mind me :) Lets still wait on the 
dependency patch before updating.




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:296
+def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
+  HelpText<"Check constraints of arguments of C standard library functions">,
+  Dependencies<[StdCLibraryFunctionsChecker]>,

martong wrote:
> Szelethus wrote:
> > How about we add an example as well?
> You mean like NonNull or other constraints?
Like
```
Check constraints of arguments of C standard library functions, such as whether 
the parameter of isalpha is in the range [0, 255] or is EOF.
```



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:161
+
+ValueRange negate() const {
+  ValueRange tmp(*this);

martong wrote:
> Szelethus wrote:
> > Maybe `complement` would be a better name? That sounds a lot more like a 
> > set operation. Also, this function highlights well that inheritance might 
> > not be the best solution here.
> Well, we check the argument constraint validity by trying to apply it's 
> logical negation. In case of a range inclusion this is being out of that 
> range. In case of non-null this is being null. And so on. The logic how we 
> try to check an argument constraint is the same in all cases of the different 
> constraints. And that is the point: in order to support a new kind of 
> constraint we just have to figure out how to "apply" and "negate" one 
> constraint. In my opinion this is a perfect case for polimorphism.
We agreed on inheritance in the previous patch, and regarding the name, sure, 
leave it as-is. :)



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:92-93
 
+  class ValueConstraint;
+  using ValueConstraintPtr = std::shared_ptr;
   class ValueConstraint {

How about `ValueConstraintRef`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898



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

[PATCH] D75063: [analyzer] StdLibraryFunctionsChecker: Add NotNull Arg Constraint

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

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:175
+using ValueConstraint::ValueConstraint;
+bool CannotBeNull = true;
+

What does this do? Is it ever used in the patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75063



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

[PATCH] D75360: [analyzer][NFC] Tie CheckerRegistry to CheckerManager, allow CheckerManager to be constructed for non-analysis purposes

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

baloghadamsoftware added a comment.

I do not say I fully grab every single character, but basically it looks good 
to me. @NoQ, please take a look on this, you are more competent in finding 
issues with it. This patch is needed to my patch preventing registration of 
checkers that depend on an analyzer option. Thx!




Comment at: clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:217
+mgr.template registerChecker();
   }
 

Why do we need `MGR` here as template parameter? Is this function also used for 
other class instances than `CheckerManager`? I fail to see such invocation.


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

https://reviews.llvm.org/D75360



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

Re: [PATCH] D65994: Extended FPOptions with new attributes

[Sam McCall via cfe-commits]

On Wed, Mar 11, 2020 at 4:09 PM Serge Pavlov  wrote:

>In this environment, 5 bits are quite a lot for FP flexibility, and I
>> think the complexity of reducing this is small and isolated (rounding +
>> exceptions together fit in 4 bits)
>
>
> Rounding (5 standard variants) and exception (3 variants) already do not
> fit 4 bits.
>
Sure they do: there are 5 x 3 = 15 combinations, which can be encoded in 4
bits. This would be a short-term fix only as it sounds like the number of
bits will grow further.

Fortunately Melanie is working on a better/more scalable solution of
putting the FPOptions in trailing objects of the nodes where they're
actually used (e.g. most BinaryOperators aren't floating point at all).
Then FPOptions can grow as many bits as needed without affecting many nodes.


> And there is also precision, denormals treatment, exception mask, they are
> usually represented in hardware register and
> make up a floating point environment. Instead of putting all these bits to
> different places it would be better to collect them in one place, it could
> make operations simpler and faster.
>
>   both rounding and exceptions seem essentially unused at present.
>
>
> Implementation of advanced floating point support is in progress, they
> will be used more and more.
>
That sounds great! Sorry, I didn't mean to imply at all they were useless,
just as an uninformed person I can't reason about them by looking at uses
as they don't exist yet.

  (Note that the cost here is not 7 bits per node but 63 - once the bits
>> available in Stmt are full the stmt subclass needs to gain a member, and
>> its alignment is 8 *bytes*)
>
>
> As objects are allocated aligned, if size in not multiple of the
> alignment, some part of memory is spent uselessly anyway.
>
Right. My point is that today, BinaryOperator uses exactly all its bits.
Tomorrow, we need to add a HasError bit to Expr. So tomorrow, the marginal
cost of the last bit (or savings from squeezing the last bit) is 64 bits,
at least for BinOp. This is certainly a short-term view, the long-term
option is probably moving FPOptions out of the bitfields in some way.

Use cases vary, and many tools deal with large ASTs but don't use LLVM
>> codegen at all.
>
>
> That is true, but developer tools are usually less restricted in computing
> resources. Besides hard economy in memory often means some loss of
> performance.
>
I'm not sure this is productive - if you don't think bit-packing is a
sensible tradeoff, then I think to some extent it's on you to change it,
rather than eat all the bits and walk away! I do happen to work on
developer tools with memory constraints, but that's not the hat I'm wearing
today - we're just stuck between the established design (which aims for
compactness) and existing uneconomical use of it.

FPOption could be shared between user using something like shared_ptr, but
>>> it means expenses of 64 bit for a pointer. Don't know if it makes sense...
>>> As you say, I don't think this saves anything, unless we can stop
>>> storing the pointer in BinaryOperator.
>>
>>
> We could keep pointer to shared properties, thus saving memory. Moreover
> such facility could enable some techniques using this shared property as a
> marker. For example, all floating point operations obtained from the same
> region where `pragma STDC FENV_ACCESS` is in act could point to the same
> FPOption object, thus we could distinguish between different regions. It
> would save memory consumption without sacrifice of maintainability and
> speed.
>
Yeah, just concretely needing an 8-byte pointer to a 1-byte shared
structure doesn't save memory, so some larger refactoring/design would be
needed. TrailingObjects is simpler.


> Thanks,
> --Serge
>
>
> On Wed, Mar 11, 2020 at 8:01 PM Sam McCall  wrote:
>
>> On Wed, Mar 11, 2020 at 12:42 PM Serge Pavlov 
>> wrote:
>>
>>> It is a matter of taste, but for me it looks strange to develop complex
>>> and error-prone system to save 7 bits at expense of readability and
>>> maintainability. My observations show that clang AST consumes much less
>>> memory than llvm objects.
>>>
>> I agree, and would be happy if we had a design without these scarce bits,
>> but AFAICS we don't. In this environment, 5 bits are quite a lot for FP
>> flexibility, and I think the complexity of reducing this is small and
>> isolated (rounding + exceptions together fit in 4 bits). But I ask about
>> the domain because maybe there's a simple but smaller model I can't really
>> infer this myself, both rounding and exceptions seem essentially unused at
>> present.
>> (Note that the cost here is not 7 bits per node but 63 - once the bits
>> available in Stmt are full the stmt subclass needs to gain a member, and
>> its alignment is 8 *bytes*)
>>
>> Use cases vary, and many tools deal with large ASTs but don't use LLVM
>> codegen at all.
>>
>> FPOption could be shared between user using something like shared_ptr,
>>> but it means expenses of 64 bit for a pointe

[PATCH] D75996: [clangd] Populate PreambleData::CompileCommand and make use of it inside buildPreamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
javed.absar, ilya-biryukov.
Herald added a project: clang.
kadircet added a parent revision: D75995: [clangd] Decouple preamble and 
mainfile builds, NFCI.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75996

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -67,8 +67,7 @@
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
   buildPreamble(FullFilename, *CI,
-/*OldPreamble=*/nullptr,
-/*OldCompileCommand=*/Inputs.CompileCommand, Inputs,
+/*OldPreamble=*/nullptr, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST =
   buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -286,16 +286,16 @@
 
   FileIndex Index;
   bool IndexUpdated = false;
-  buildPreamble(
-  FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
-  /*StoreInMemory=*/true,
-  [&](ASTContext &Ctx, std::shared_ptr PP,
-  const CanonicalIncludes &CanonIncludes) {
-EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
-IndexUpdated = true;
-Index.updatePreamble(FooCpp, /*Version=*/"null", Ctx, std::move(PP),
- CanonIncludes);
-  });
+  buildPreamble(FooCpp, *CI, /*OldPreamble=*/nullptr, PI,
+/*StoreInMemory=*/true,
+[&](ASTContext &Ctx, std::shared_ptr PP,
+const CanonicalIncludes &CanonIncludes) {
+  EXPECT_FALSE(IndexUpdated)
+  << "Expected only a single index update";
+  IndexUpdated = true;
+  Index.updatePreamble(FooCpp, /*Version=*/"null", Ctx,
+   std::move(PP), CanonIncludes);
+});
   ASSERT_TRUE(IndexUpdated);
 
   // Check the index contains symbols from the preamble, but not from the main
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -331,16 +331,12 @@
 std::shared_ptr OldPreamble =
 Inputs.ForceRebuild ? std::shared_ptr()
 : getLatestBuiltPreamble();
-// FIXME: We should rather use the compile command in the OldPreamble, but
-// currently it is not populated.
-const tooling::CompileCommand OldCmd = LastCmd;
-LastCmd = Inputs.CompileCommand;
 
 std::string TaskName = llvm::formatv("Update ({0})", Inputs.Version);
 emitTUStatus({TUAction::BuildingPreamble, std::move(TaskName)});
 
 return clang::clangd::buildPreamble(
-FileName, std::move(*Req.CI), OldPreamble, OldCmd, Inputs,
+FileName, std::move(*Req.CI), OldPreamble, Inputs,
 StorePreambleInMemory,
 [this, Version(Inputs.Version)](
 ASTContext &Ctx, std::shared_ptr PP,
@@ -362,9 +358,6 @@
   std::shared_ptr LastBuiltPreamble;
 
   Notification BuiltFirstPreamble;
-
-  // Only used by current worker thread, so no data races.
-  tooling::CompileCommand LastCmd;
   const bool StorePreambleInMemory;
 };
 
Index: clang-tools-extra/clangd/Preamble.h
===
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -43,7 +43,7 @@
 /// As we must avoid re-parsing the preamble, any information that can only
 /// be obtained during parsing must be eagerly captured and stored here.
 struct PreambleData {
-  PreambleData(llvm::StringRef Version, PrecompiledPreamble Preamble,
+  PreambleData(const ParseInputs &Inputs, PrecompiledPreamble Preamble,
std::vector Diags, IncludeStructure Includes,
MainFileMacros Macros,
std::unique_ptr StatCache,
@@ -80,7 +80,6 @@
 std::shared_ptr
 buildPreamble(PathRef FileName, CompilerInvocation CI,
   std::shared_ptr OldPreamble,
-  const tooling::CompileCommand &OldCompileCommand,
   const ParseInputs &Inputs, bool StoreInMemory,
   PreambleParsedCallback PreambleCallback);
 

[PATCH] D75997: [ARM,MVE] Fix user-namespace violation in arm_mve.h.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham created this revision.
simon_tatham added reviewers: miyuki, MarkMurrayARM, ostannard.
Herald added subscribers: cfe-commits, danielkiss, dmgreen, kristof.beyls.
Herald added a project: clang.

We were generating the declarations of polymorphic intrinsics using
`__attribute__((overloadable))`. But `overloadable` is a valid
identifier for an end user to define as a macro in a C program, and if
they do that before including ``, then we shouldn't cause a
compile error.

Fixed to spell the attribute name `__overloadable__` instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75997

Files:
  clang/utils/TableGen/MveEmitter.cpp


Index: clang/utils/TableGen/MveEmitter.cpp
===
--- clang/utils/TableGen/MveEmitter.cpp
+++ clang/utils/TableGen/MveEmitter.cpp
@@ -1874,7 +1874,7 @@
 // match your call".
 
 OS << "static __inline__ __attribute__(("
-   << (Polymorphic ? "overloadable, " : "")
+   << (Polymorphic ? "__overloadable__, " : "")
<< "__clang_arm_builtin_alias(__builtin_arm_mve_" << Int.fullName()
<< ")))\n"
<< RetTypeName << FunctionName << "(" << ArgTypesString << ");\n";
@@ -2041,7 +2041,7 @@
   // Emit the actual declaration. See MveEmitter::EmitHeader for detailed
   // comments
   OS << "static __inline__ __attribute__(("
- << (Polymorphic ? "overloadable, " : "")
+ << (Polymorphic ? "__overloadable__, " : "")
  << "__clang_arm_builtin_alias(__builtin_arm_" << 
Int.builtinExtension()
  << "_" << Int.fullName() << ")))\n"
  << RetTypeName << FunctionName << "(" << ArgTypesString << ");\n";


Index: clang/utils/TableGen/MveEmitter.cpp
===
--- clang/utils/TableGen/MveEmitter.cpp
+++ clang/utils/TableGen/MveEmitter.cpp
@@ -1874,7 +1874,7 @@
 // match your call".
 
 OS << "static __inline__ __attribute__(("
-   << (Polymorphic ? "overloadable, " : "")
+   << (Polymorphic ? "__overloadable__, " : "")
<< "__clang_arm_builtin_alias(__builtin_arm_mve_" << Int.fullName()
<< ")))\n"
<< RetTypeName << FunctionName << "(" << ArgTypesString << ");\n";
@@ -2041,7 +2041,7 @@
   // Emit the actual declaration. See MveEmitter::EmitHeader for detailed
   // comments
   OS << "static __inline__ __attribute__(("
- << (Polymorphic ? "overloadable, " : "")
+ << (Polymorphic ? "__overloadable__, " : "")
  << "__clang_arm_builtin_alias(__builtin_arm_" << Int.builtinExtension()
  << "_" << Int.fullName() << ")))\n"
  << RetTypeName << FunctionName << "(" << ArgTypesString << ");\n";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75998: [ARM,MVE] Add intrinsics and isel for MVE fused multiply-add.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham created this revision.
simon_tatham added reviewers: dmgreen, MarkMurrayARM, miyuki, ostannard.
Herald added subscribers: llvm-commits, cfe-commits, danielkiss, hiraditya, 
kristof.beyls.
Herald added projects: clang, LLVM.
simon_tatham updated this revision to Diff 249641.
simon_tatham added a comment.

Update test results following a last-minute correctness fix. (I updated one of 
the two test files but not the other.)


This adds the ACLE intrinsic family for the VFMA and VFMS
instructions, which perform fused multiply-add on vectors of floats.

I've represented the unpredicated versions in IR using the cross-
platform `@llvm.fma` IR intrinsic. We already had isel rules to
convert one of those into a vector VFMA in the simplest possible way;
but we didn't have rules to detect a negated argument and turn it into
VFMS, or rules to detect a splat argument and turn it into one of the
two vector/scalar forms of the instruction. Now we have all of those.

The predicated form uses a target-specific intrinsic as usual, but
I've stuck to just one, for a predicated FMA. The subtraction and
splat versions are code-generated by passing an fneg or a splat as one
of its operands, the same way as the unpredicated version.

In arm_mve_defs.h, I've had to introduce a tiny extra piece of
infrastructure: a record `id` for use in codegen dags which implements
the identity function. (Just because you can't declare a Tablegen
value of type dag which is //only// a `$varname`: you have to wrap it
in something. Now I can write `(id $varname)` to get the same effect.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75998

Files:
  clang/include/clang/Basic/arm_mve.td
  clang/include/clang/Basic/arm_mve_defs.td
  clang/test/CodeGen/arm-mve-intrinsics/ternary.c
  llvm/include/llvm/IR/IntrinsicsARM.td
  llvm/lib/Target/ARM/ARMInstrMVE.td
  llvm/test/CodeGen/Thumb2/mve-fmas.ll
  llvm/test/CodeGen/Thumb2/mve-intrinsics/ternary.ll

Index: llvm/test/CodeGen/Thumb2/mve-intrinsics/ternary.ll
===
--- /dev/null
+++ llvm/test/CodeGen/Thumb2/mve-intrinsics/ternary.ll
@@ -0,0 +1,242 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=thumbv8.1m.main -mattr=+mve.fp -verify-machineinstrs -o - %s | FileCheck %s
+
+define arm_aapcs_vfpcc <8 x half> @test_vfmaq_f16(<8 x half> %a, <8 x half> %b, <8 x half> %c) {
+; CHECK-LABEL: test_vfmaq_f16:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vfma.f16 q0, q1, q2
+; CHECK-NEXT:bx lr
+entry:
+  %0 = tail call <8 x half> @llvm.fma.v8f16(<8 x half> %b, <8 x half> %c, <8 x half> %a)
+  ret <8 x half> %0
+}
+
+define arm_aapcs_vfpcc <4 x float> @test_vfmaq_f32(<4 x float> %a, <4 x float> %b, <4 x float> %c) {
+; CHECK-LABEL: test_vfmaq_f32:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vfma.f32 q0, q1, q2
+; CHECK-NEXT:bx lr
+entry:
+  %0 = tail call <4 x float> @llvm.fma.v4f32(<4 x float> %b, <4 x float> %c, <4 x float> %a)
+  ret <4 x float> %0
+}
+
+define arm_aapcs_vfpcc <8 x half> @test_vfmaq_n_f16(<8 x half> %a, <8 x half> %b, float %c.coerce) {
+; CHECK-LABEL: test_vfmaq_n_f16:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vmov r0, s8
+; CHECK-NEXT:vfma.f16 q0, q1, r0
+; CHECK-NEXT:bx lr
+entry:
+  %0 = bitcast float %c.coerce to i32
+  %tmp.0.extract.trunc = trunc i32 %0 to i16
+  %1 = bitcast i16 %tmp.0.extract.trunc to half
+  %.splatinsert = insertelement <8 x half> undef, half %1, i32 0
+  %.splat = shufflevector <8 x half> %.splatinsert, <8 x half> undef, <8 x i32> zeroinitializer
+  %2 = tail call <8 x half> @llvm.fma.v8f16(<8 x half> %b, <8 x half> %.splat, <8 x half> %a)
+  ret <8 x half> %2
+}
+
+define arm_aapcs_vfpcc <4 x float> @test_vfmaq_n_f32(<4 x float> %a, <4 x float> %b, float %c) {
+; CHECK-LABEL: test_vfmaq_n_f32:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vmov r0, s8
+; CHECK-NEXT:vfma.f32 q0, q1, r0
+; CHECK-NEXT:bx lr
+entry:
+  %.splatinsert = insertelement <4 x float> undef, float %c, i32 0
+  %.splat = shufflevector <4 x float> %.splatinsert, <4 x float> undef, <4 x i32> zeroinitializer
+  %0 = tail call <4 x float> @llvm.fma.v4f32(<4 x float> %b, <4 x float> %.splat, <4 x float> %a)
+  ret <4 x float> %0
+}
+
+define arm_aapcs_vfpcc <8 x half> @test_vfmasq_n_f16(<8 x half> %a, <8 x half> %b, float %c.coerce) {
+; CHECK-LABEL: test_vfmasq_n_f16:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vmov r0, s8
+; CHECK-NEXT:vfmas.f16 q0, q1, r0
+; CHECK-NEXT:bx lr
+entry:
+  %0 = bitcast float %c.coerce to i32
+  %tmp.0.extract.trunc = trunc i32 %0 to i16
+  %1 = bitcast i16 %tmp.0.extract.trunc to half
+  %.splatinsert = insertelement <8 x half> undef, half %1, i32 0
+  %.splat = shufflevector <8 x half> %.splatinsert, <8 x half> undef, <8 x i32> zeroinitializer
+  %2 = tail call <8 x half> @llvm.fma.v8f16(<8 x half> %a, <8 x half> %b, <8 x half> %.splat)
+  ret <8 x half> 

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 249642.
balazske added a comment.

- Removed AnyError.
- Using common code for `evalFeof` and ferror.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -19,7 +19,6 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
-#include 
 
 using namespace clang;
 using namespace ento;
@@ -30,6 +29,7 @@
 /// Full state information about a stream pointer.
 struct StreamState {
   /// State of a stream symbol.
+  /// FIXME: use a bool isOpened instead.
   enum KindTy {
 Opened, /// Stream is opened.
 Closed, /// Closed stream (an invalid stream pointer after it was closed).
@@ -37,37 +37,44 @@
   } State;
 
   /// The error state of a stream.
+  /// Valid only if the stream is opened.
   enum ErrorKindTy {
-NoError,/// No error flag is set or stream is not open.
-EofError,   /// EOF condition (`feof` is true).
-OtherError, /// Other (non-EOF) error (`ferror` is true).
-AnyError/// EofError or OtherError, used if exact error is unknown.
+/// No error flag is set (or stream is not open).
+NoError,
+/// EOF condition (`feof` is true).
+FEof,
+/// Other generic (non-EOF) error (`ferror` is true).
+FError,
   } ErrorState = NoError;
 
   bool isOpened() const { return State == Opened; }
   bool isClosed() const { return State == Closed; }
   bool isOpenFailed() const { return State == OpenFailed; }
 
-  bool isNoError() const { return ErrorState == NoError; }
-  bool isEofError() const { return ErrorState == EofError; }
-  bool isOtherError() const { return ErrorState == OtherError; }
-  bool isAnyError() const { return ErrorState == AnyError; }
+  bool isNoError() const {
+assert(State == Opened && "Error undefined for closed stream.");
+return ErrorState == NoError;
+  }
+  bool isFEof() const {
+assert(State == Opened && "Error undefined for closed stream.");
+return ErrorState == FEof;
+  }
+  bool isFError() const {
+assert(State == Opened && "Error undefined for closed stream.");
+return ErrorState == FError;
+  }
 
   bool operator==(const StreamState &X) const {
+// In not opened state error should always NoError.
 return State == X.State && ErrorState == X.ErrorState;
   }
 
   static StreamState getOpened() { return StreamState{Opened}; }
   static StreamState getClosed() { return StreamState{Closed}; }
   static StreamState getOpenFailed() { return StreamState{OpenFailed}; }
-  static StreamState getOpenedWithAnyError() {
-return StreamState{Opened, AnyError};
-  }
-  static StreamState getOpenedWithEofError() {
-return StreamState{Opened, EofError};
-  }
-  static StreamState getOpenedWithOtherError() {
-return StreamState{Opened, OtherError};
+  static StreamState getOpenedWithFEof() { return StreamState{Opened, FEof}; }
+  static StreamState getOpenedWithFError() {
+return StreamState{Opened, FError};
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const {
@@ -135,9 +142,12 @@
   {{"fsetpos", 2}, {&StreamChecker::preDefault, nullptr, 0}},
   {{"clearerr", 1},
{&StreamChecker::preDefault, &StreamChecker::evalClearerr, 0}},
-  {{"feof", 1}, {&StreamChecker::preDefault, &StreamChecker::evalFeof, 0}},
+  {{"feof", 1},
+   {&StreamChecker::preDefault,
+&StreamChecker::evalFeofFerror<&StreamState::isFEof>, 0}},
   {{"ferror", 1},
-   {&StreamChecker::preDefault, &StreamChecker::evalFerror, 0}},
+   {&StreamChecker::preDefault,
+&StreamChecker::evalFeofFerror<&StreamState::isFError>, 0}},
   {{"fileno", 1}, {&StreamChecker::preDefault, nullptr, 0}},
   };
 
@@ -161,11 +171,9 @@
   void evalClearerr(const FnDescription *Desc, const CallEvent &Call,
 CheckerContext &C) const;
 
-  void evalFeof(const FnDescription *Desc, const CallEvent &Call,
-CheckerContext &C) const;
-
-  void evalFerror(const FnDescription *Desc, const CallEvent &Call,
-  CheckerContext &C) const;
+  template 
+  void evalFeofFerror(const FnDescription *Desc, const CallEvent &Call,
+  CheckerContext &C) const;
 
   /// Check that the stream (in StreamVal) is not NULL.
   /// If it can only be NULL a fatal error is emitted and nullptr returned.
@@ -359,8 +367,10 @@
   C.addTransition(State);
 }
 
-void StreamChecker::evalFeof(const FnDescription *Desc, const CallEvent &Call,
- CheckerContext &C) const {
+tem

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

[Sam McCall via Phabricator via cfe-commits]

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

In D72874#1915840 , @nridge wrote:

> I've started to update the patch to be in line with the direction discussed 
> in the issue.
>
> @sammccall, how would you like to proceed logistically:
>
> - Do you plan to land (a possibly modified version of) D75479 
> ?
> - Or should I combine that patch into this one?


This patch looks good, I wouldn't bother redesigning anything, we should 
iterate instead.

You should go ahead, and I'll merge, and then we should work towards enabling 
dependent code use cases etc. SG?




Comment at: clang-tools-extra/clangd/FindSymbols.h:25
+/// Helper function for deriving an LSP Location from a SymbolLocation.
+llvm::Expected symbolLocationToLocation(const SymbolLocation &Loc,
+  llvm::StringRef HintPath);

nit: these names are vague and echo the type signature, maybe 
indexToLSPLocation?



Comment at: clang-tools-extra/clangd/FindSymbols.h:26
+llvm::Expected symbolLocationToLocation(const SymbolLocation &Loc,
+  llvm::StringRef HintPath);
+

nit: HintPath should be TUPath, the decision to use some other path as a TU 
path can only be made in the caller (needs context).
(Same is true for symbolToLocation, I'm not sure when that became public)



Comment at: clang-tools-extra/clangd/XRefs.cpp:486
+  Req.Limit = 10;
+  TopN Top(*Req.Limit);
+  FuzzyMatcher Filter(Req.Query);

(The fuzzy matcher and topN are still here - I think we don't need them, right? 
With only up-to-3 results, std::sort seems more obvious)



Comment at: clang-tools-extra/clangd/XRefs.cpp:488
+  FuzzyMatcher Filter(Req.Query);
+  Index->fuzzyFind(Req, [&](const Symbol &Sym) {
+auto MaybeDeclLoc =

maybe bail out early (on unusable/too many) instead of doing all the score 
computations first?
fuzzyFind(..., {
  // bail out if it's a constructor or name doesn't match
  if (Results.size() >= 3) {
TooMany = true;
return;
  }
  // add result
});


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72874



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 249643.
balazske added a comment.

Adding correct diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/stream-error.c
  clang/test/Analysis/stream.c

Index: clang/test/Analysis/stream.c
===
--- clang/test/Analysis/stream.c
+++ clang/test/Analysis/stream.c
@@ -1,26 +1,6 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -verify %s
 
-typedef __typeof__(sizeof(int)) size_t;
-typedef __typeof__(sizeof(int)) fpos_t;
-typedef struct _IO_FILE FILE;
-#define SEEK_SET  0  /* Seek from beginning of file.  */
-#define SEEK_CUR  1  /* Seek from current position.  */
-#define SEEK_END  2  /* Seek from end of file.  */
-extern FILE *fopen(const char *path, const char *mode);
-extern FILE *tmpfile(void);
-extern int fclose(FILE *fp);
-extern size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
-extern size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
-extern int fseek (FILE *__stream, long int __off, int __whence);
-extern long int ftell (FILE *__stream);
-extern void rewind (FILE *__stream);
-extern int fgetpos(FILE *stream, fpos_t *pos);
-extern int fsetpos(FILE *stream, const fpos_t *pos);
-extern void clearerr(FILE *stream);
-extern int feof(FILE *stream);
-extern int ferror(FILE *stream);
-extern int fileno(FILE *stream);
-extern FILE *freopen(const char *pathname, const char *mode, FILE *stream);
+#include "Inputs/system-header-simulator.h"
 
 void check_fread() {
   FILE *fp = tmpfile();
Index: clang/test/Analysis/stream-error.c
===
--- /dev/null
+++ clang/test/Analysis/stream-error.c
@@ -0,0 +1,40 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-checker=debug.ExprInspection -analyzer-store region -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_eval(int);
+
+void error_fopen() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  fclose(F);
+}
+
+void error_freopen() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  F = freopen(0, "w", F);
+  if (!F)
+return;
+  clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  fclose(F);
+}
+
+void error_clearerr() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+return;
+  int ch = fputc('a', F);
+  if (ch == EOF) {
+// FIXME: fputc not handled by checker yet, should expect TRUE
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+clearerr(F);
+clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  }
+  fclose(F);
+}
Index: clang/test/Analysis/Inputs/system-header-simulator.h
===
--- clang/test/Analysis/Inputs/system-header-simulator.h
+++ clang/test/Analysis/Inputs/system-header-simulator.h
@@ -9,7 +9,16 @@
 #define restrict /*restrict*/
 #endif
 
+typedef __typeof(sizeof(int)) size_t;
+typedef long long __int64_t;
+typedef __int64_t __darwin_off_t;
+typedef __darwin_off_t fpos_t;
+
 typedef struct _FILE FILE;
+#define SEEK_SET 0 /* Seek from beginning of file. */
+#define SEEK_CUR 1 /* Seek from current position. */
+#define SEEK_END 2 /* Seek from end of file. */
+
 extern FILE *stdin;
 extern FILE *stdout;
 extern FILE *stderr;
@@ -24,11 +33,35 @@
 int fprintf(FILE *restrict, const char *restrict, ...);
 int getchar(void);
 
+void setbuf(FILE *restrict, char *restrict);
+int setvbuf(FILE *restrict, char *restrict, int, size_t);
+
+FILE *funopen(const void *,
+  int (*)(void *, char *, int),
+  int (*)(void *, const char *, int),
+  fpos_t (*)(void *, fpos_t, int),
+  int (*)(void *));
+
+FILE *fopen(const char *path, const char *mode);
+FILE *tmpfile(void);
+FILE *freopen(const char *pathname, const char *mode, FILE *stream);
+int fclose(FILE *fp);
+size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
+size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
+int fputc(int ch, FILE *stream);
+int fseek(FILE *__stream, long int __off, int __whence);
+long int ftell(FILE *__stream);
+void rewind(FILE *__stream);
+int fgetpos(FILE *stream, fpos_t *pos);
+int fsetpos(FILE *stream, const fpos_t *pos);
+void clearerr(FILE *stream);
+int feof(FILE *stream);
+int ferror(FILE *stream);
+int fileno(FILE *stream);
+
 // Note, on some platforms errno macro gets replaced with a function call.
 extern int errno;
 
-typedef __typeof(sizeof(i

[PATCH] D75998: [ARM,MVE] Add intrinsics and isel for MVE fused multiply-add.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham updated this revision to Diff 249641.
simon_tatham added a comment.

Update test results following a last-minute correctness fix. (I updated one of 
the two test files but not the other.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75998

Files:
  clang/include/clang/Basic/arm_mve.td
  clang/include/clang/Basic/arm_mve_defs.td
  clang/test/CodeGen/arm-mve-intrinsics/ternary.c
  llvm/include/llvm/IR/IntrinsicsARM.td
  llvm/lib/Target/ARM/ARMInstrMVE.td
  llvm/test/CodeGen/Thumb2/mve-fmas.ll
  llvm/test/CodeGen/Thumb2/mve-intrinsics/ternary.ll

Index: llvm/test/CodeGen/Thumb2/mve-intrinsics/ternary.ll
===
--- /dev/null
+++ llvm/test/CodeGen/Thumb2/mve-intrinsics/ternary.ll
@@ -0,0 +1,242 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=thumbv8.1m.main -mattr=+mve.fp -verify-machineinstrs -o - %s | FileCheck %s
+
+define arm_aapcs_vfpcc <8 x half> @test_vfmaq_f16(<8 x half> %a, <8 x half> %b, <8 x half> %c) {
+; CHECK-LABEL: test_vfmaq_f16:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vfma.f16 q0, q1, q2
+; CHECK-NEXT:bx lr
+entry:
+  %0 = tail call <8 x half> @llvm.fma.v8f16(<8 x half> %b, <8 x half> %c, <8 x half> %a)
+  ret <8 x half> %0
+}
+
+define arm_aapcs_vfpcc <4 x float> @test_vfmaq_f32(<4 x float> %a, <4 x float> %b, <4 x float> %c) {
+; CHECK-LABEL: test_vfmaq_f32:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vfma.f32 q0, q1, q2
+; CHECK-NEXT:bx lr
+entry:
+  %0 = tail call <4 x float> @llvm.fma.v4f32(<4 x float> %b, <4 x float> %c, <4 x float> %a)
+  ret <4 x float> %0
+}
+
+define arm_aapcs_vfpcc <8 x half> @test_vfmaq_n_f16(<8 x half> %a, <8 x half> %b, float %c.coerce) {
+; CHECK-LABEL: test_vfmaq_n_f16:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vmov r0, s8
+; CHECK-NEXT:vfma.f16 q0, q1, r0
+; CHECK-NEXT:bx lr
+entry:
+  %0 = bitcast float %c.coerce to i32
+  %tmp.0.extract.trunc = trunc i32 %0 to i16
+  %1 = bitcast i16 %tmp.0.extract.trunc to half
+  %.splatinsert = insertelement <8 x half> undef, half %1, i32 0
+  %.splat = shufflevector <8 x half> %.splatinsert, <8 x half> undef, <8 x i32> zeroinitializer
+  %2 = tail call <8 x half> @llvm.fma.v8f16(<8 x half> %b, <8 x half> %.splat, <8 x half> %a)
+  ret <8 x half> %2
+}
+
+define arm_aapcs_vfpcc <4 x float> @test_vfmaq_n_f32(<4 x float> %a, <4 x float> %b, float %c) {
+; CHECK-LABEL: test_vfmaq_n_f32:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vmov r0, s8
+; CHECK-NEXT:vfma.f32 q0, q1, r0
+; CHECK-NEXT:bx lr
+entry:
+  %.splatinsert = insertelement <4 x float> undef, float %c, i32 0
+  %.splat = shufflevector <4 x float> %.splatinsert, <4 x float> undef, <4 x i32> zeroinitializer
+  %0 = tail call <4 x float> @llvm.fma.v4f32(<4 x float> %b, <4 x float> %.splat, <4 x float> %a)
+  ret <4 x float> %0
+}
+
+define arm_aapcs_vfpcc <8 x half> @test_vfmasq_n_f16(<8 x half> %a, <8 x half> %b, float %c.coerce) {
+; CHECK-LABEL: test_vfmasq_n_f16:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vmov r0, s8
+; CHECK-NEXT:vfmas.f16 q0, q1, r0
+; CHECK-NEXT:bx lr
+entry:
+  %0 = bitcast float %c.coerce to i32
+  %tmp.0.extract.trunc = trunc i32 %0 to i16
+  %1 = bitcast i16 %tmp.0.extract.trunc to half
+  %.splatinsert = insertelement <8 x half> undef, half %1, i32 0
+  %.splat = shufflevector <8 x half> %.splatinsert, <8 x half> undef, <8 x i32> zeroinitializer
+  %2 = tail call <8 x half> @llvm.fma.v8f16(<8 x half> %a, <8 x half> %b, <8 x half> %.splat)
+  ret <8 x half> %2
+}
+
+define arm_aapcs_vfpcc <4 x float> @test_vfmasq_n_f32(<4 x float> %a, <4 x float> %b, float %c) {
+; CHECK-LABEL: test_vfmasq_n_f32:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vmov r0, s8
+; CHECK-NEXT:vfmas.f32 q0, q1, r0
+; CHECK-NEXT:bx lr
+entry:
+  %.splatinsert = insertelement <4 x float> undef, float %c, i32 0
+  %.splat = shufflevector <4 x float> %.splatinsert, <4 x float> undef, <4 x i32> zeroinitializer
+  %0 = tail call <4 x float> @llvm.fma.v4f32(<4 x float> %a, <4 x float> %b, <4 x float> %.splat)
+  ret <4 x float> %0
+}
+
+define arm_aapcs_vfpcc <8 x half> @test_vfmsq_f16(<8 x half> %a, <8 x half> %b, <8 x half> %c) {
+; CHECK-LABEL: test_vfmsq_f16:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vfms.f16 q0, q1, q2
+; CHECK-NEXT:bx lr
+entry:
+  %0 = fneg <8 x half> %c
+  %1 = tail call <8 x half> @llvm.fma.v8f16(<8 x half> %b, <8 x half> %0, <8 x half> %a)
+  ret <8 x half> %1
+}
+
+define arm_aapcs_vfpcc <4 x float> @test_vfmsq_f32(<4 x float> %a, <4 x float> %b, <4 x float> %c) {
+; CHECK-LABEL: test_vfmsq_f32:
+; CHECK:   @ %bb.0: @ %entry
+; CHECK-NEXT:vfms.f32 q0, q1, q2
+; CHECK-NEXT:bx lr
+entry:
+  %0 = fneg <4 x float> %c
+  %1 = tail call <4 x float> @llvm.fma.v4f32(<4 x float> %b, <4 x float> %0, <4 x float> %a)
+  ret <4 x

[PATCH] D75985: clangd doc: Show a test case for clangd with some commands

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks Sylvestre!

We're actually in the process of moving docs to a new site (preview: 
https://clangd.github.io - once set up it will be at clangd.llvm.org).

Who do you think is the audience/purpose for this doc? End-users don't need to 
understand this to use clangd (hopefully!).
On the other hand, they are developers, and "roughly how does this work" is a 
pretty good question.

Maybe we can include an incomplete snippet (e.g. abbreviated open, completion, 
response) on the front page or on /design/?
Some of the details (`-lit-test` flag, `test` uri scheme) should probably be 
changed unless the purpose really is to explain the lit-testtest setup 
specifically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75985



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

[PATCH] D76001: [AST] Respect shouldTraversePostOrder when traversing type locs

[Marcel Hlopko via Phabricator via cfe-commits]

hlopko created this revision.
hlopko added a reviewer: gribozavr2.
hlopko added a project: clang.
Herald added a subscriber: cfe-commits.

Copy of https://reviews.llvm.org/D72072, submitting with ilya-biryukov's 
permission.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76001

Files:
  clang/include/clang/AST/RecursiveASTVisitor.h


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -1127,10 +1127,17 @@
 #define DEF_TRAVERSE_TYPELOC(TYPE, CODE)   
\
   template   
\
   bool RecursiveASTVisitor::Traverse##TYPE##Loc(TYPE##Loc TL) {   
\
-if (getDerived().shouldWalkTypesOfTypeLocs())  
\
-  TRY_TO(WalkUpFrom##TYPE(const_cast(TL.getTypePtr(;   
\
-TRY_TO(WalkUpFrom##TYPE##Loc(TL)); 
\
+if (!getDerived().shouldTraversePostOrder()) { 
\
+  TRY_TO(WalkUpFrom##TYPE##Loc(TL));   
\
+  if (getDerived().shouldWalkTypesOfTypeLocs())
\
+TRY_TO(WalkUpFrom##TYPE(const_cast(TL.getTypePtr(; 
\
+}  
\
 { CODE; }  
\
+if (getDerived().shouldTraversePostOrder()) {  
\
+  TRY_TO(WalkUpFrom##TYPE##Loc(TL));   
\
+  if (getDerived().shouldWalkTypesOfTypeLocs())
\
+TRY_TO(WalkUpFrom##TYPE(const_cast(TL.getTypePtr(; 
\
+}  
\
 return true;   
\
   }
 
@@ -1199,22 +1206,22 @@
 
 DEF_TRAVERSE_TYPELOC(ConstantArrayType, {
   TRY_TO(TraverseTypeLoc(TL.getElementLoc()));
-  return TraverseArrayTypeLocHelper(TL);
+  TRY_TO(TraverseArrayTypeLocHelper(TL));
 })
 
 DEF_TRAVERSE_TYPELOC(IncompleteArrayType, {
   TRY_TO(TraverseTypeLoc(TL.getElementLoc()));
-  return TraverseArrayTypeLocHelper(TL);
+  TRY_TO(TraverseArrayTypeLocHelper(TL));
 })
 
 DEF_TRAVERSE_TYPELOC(VariableArrayType, {
   TRY_TO(TraverseTypeLoc(TL.getElementLoc()));
-  return TraverseArrayTypeLocHelper(TL);
+  TRY_TO(TraverseArrayTypeLocHelper(TL));
 })
 
 DEF_TRAVERSE_TYPELOC(DependentSizedArrayType, {
   TRY_TO(TraverseTypeLoc(TL.getElementLoc()));
-  return TraverseArrayTypeLocHelper(TL);
+  TRY_TO(TraverseArrayTypeLocHelper(TL));
 })
 
 DEF_TRAVERSE_TYPELOC(DependentAddressSpaceType, {


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -1127,10 +1127,17 @@
 #define DEF_TRAVERSE_TYPELOC(TYPE, CODE)   \
   template   \
   bool RecursiveASTVisitor::Traverse##TYPE##Loc(TYPE##Loc TL) {   \
-if (getDerived().shouldWalkTypesOfTypeLocs())  \
-  TRY_TO(WalkUpFrom##TYPE(const_cast(TL.getTypePtr(;   \
-TRY_TO(WalkUpFrom##TYPE##Loc(TL)); \
+if (!getDerived().shouldTraversePostOrder()) { \
+  TRY_TO(WalkUpFrom##TYPE##Loc(TL));   \
+  if (getDerived().shouldWalkTypesOfTypeLocs())\
+TRY_TO(WalkUpFrom##TYPE(const_cast(TL.getTypePtr(; \
+}  \
 { CODE; }  \
+if (getDerived().shouldTraversePostOrder()) {  \
+  TRY_TO(WalkUpFrom##TYPE##Loc(TL));   \
+  if (getDerived().shouldWalkTypesOfTypeLocs())\
+TRY_TO(WalkUpFrom##TYPE(const_cast(TL.getTypePtr(; \
+}  \
 return true;   \
   }
 
@@ -1199,22 +1206,22 @@
 
 DEF_TRAVERSE_TYPELOC(ConstantArrayType, {
   TRY_TO(TraverseTypeLoc(TL.getElementLoc()));
-  return TraverseArrayTypeLocHelper(TL);
+  TRY_TO(TraverseArrayTypeLocHelper(TL));
 })
 
 DEF_TRAVERSE_TYPELOC(IncompleteArrayType, {
   TRY_TO(TraverseTypeLoc(TL.getElementLoc()));
-  return TraverseArrayTypeLocHelper(TL);
+  TRY_TO(TraverseArrayTypeLocHelper(TL));
 })
 
 DEF_TRAVERSE_TYPELOC(VariableArrayType, {
   TRY_TO(TraverseTyp

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

If the bit is unused except for propagation, please remove it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59214



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

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D59214#1916596 , @sammccall wrote:

> So if I understand the history here:
>
> - the `IsOMPStructuredBlock` bit on `Stmt` was added to implement 
> `getStructuredBlock()`
> - after review, `getStructuredBlock()` doesn't use the bit
> - the bit is used in the textual AST dump, and an AST matcher (which is in 
> turn never used in-tree)
>
>   Can we have the bit back please :-) We're currently trying to add a 
> HasErrors bit to improve error-recovery and some nodes are completely full. 
> Using a bit on every Stmt to improve the text dump a little and provide a 
> matcher (only relevant to OMP files) seems disproportionately expensive.




In D59214#1917106 , @rjmccall wrote:

> If the bit is unused except for propagation, please remove it.


This review (& https://bugs.llvm.org/show_bug.cgi?id=40563 in general)
has left me so burnout as to all this openmp stuff that i never
followed-up with the actual planned usages of this bit.

The reasons why this bit is needed (i think they were spelled out here)
still apply. One obvious example is https://bugs.llvm.org/show_bug.cgi?id=41102,
where after we'd teach `ExceptionAnalyzer` to look into `CapturedStmt` / 
`CapturedDecl`,
we'd immediately have false-positives for these OpenMP structured blocks,
so we'd need to avoid traversing into them.

Doing that via a single bit check should be trivial, as opposed to,
i dunno, maintainting a set of all the openmp structured block statements,
and before visiting each statement seeing if it is in that set?
That kinda sounds slow.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59214



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

[PATCH] D75985: clangd doc: Show a test case for clangd with some commands

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru updated this revision to Diff 249653.
sylvestre.ledru added a comment.

fix some typo + desc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75985

Files:
  clang-tools-extra/docs/clangd/SimpleExample.rst
  clang-tools-extra/docs/clangd/index.rst

Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -8,6 +8,7 @@
Installation
Features
Configuration
+   SimpleExample
 
 What is clangd?
 ===
Index: clang-tools-extra/docs/clangd/SimpleExample.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clangd/SimpleExample.rst
@@ -0,0 +1,110 @@
+=
+Simple example clangd
+=
+
+clangd is not designed to be used in command line.
+However, we are providing this example to show how it works.
+
+The following file (ex: commands.json) represents a list of commands
+to get a completion suggestion.
+
+.. code-block:: json
+
+   {
+ "jsonrpc": "2.0",
+ "id": 0,
+ "method": "initialize",
+ "params": {
+   "capabilities": {
+ "textDocument": {
+   "completion": {
+ "completionItem": {
+   "snippetSupport": true
+ }
+   }
+ }
+   },
+   "trace": "off"
+ }
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "method": "textDocument/didOpen",
+   "params": {
+   "textDocument": {
+   "uri": "test:///main.cpp",
+   "languageId": "cpp",
+   "version": 1,
+   "text": "int func_with_args(int a, int b);\nint main() {\nfunc_with\n}"
+   }
+   }
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "id": 1,
+   "method": "textDocument/completion",
+   "params": {
+   "textDocument": {
+   "uri": "test:///main.cpp"
+   },
+   "position": {
+   "line": 2,
+   "character": 7
+}
+}
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "id": 4,
+   "method": "shutdown"
+   }
+   ---
+   {
+   "jsonrpc": "2.0",
+   "method": "exit"
+   }
+
+
+To run all the commands at once:
+
+.. code-block:: shell
+
+   clangd -lit-test < commands.json
+
+This example will try to get the completion on line 2, column 7.
+Here is a subset of the returned information:
+
+.. code-block:: json
+
+[...]
+"items": [
+  {
+"detail": "int",
+"filterText": "func_with_args",
+"insertText": "func_with_args(${1:int a}, ${2:int b})",
+"insertTextFormat": 2,
+"kind": 3,
+"label": " func_with_args(int a, int b)",
+"score": 9.905722045898,
+"sortText": "3ee1func_with_args",
+"textEdit": {
+  "newText": "func_with_args(${1:int a}, ${2:int b})",
+  "range": {
+"end": {
+  "character": 7,
+  "line": 2
+},
+"start": {
+  "character": 0,
+  "line": 2
+}
+  }
+}
+  }
+[...]
+
+`newText` shows that clangd is able to find a suggestion for the completion and
+where to insert it.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75985: clangd doc: Show a test case for clangd with some commands

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

Good question :)
I have been wearing two hats here.

With my Debian/Ubuntu hat, i have been interested to write some dumb 
integration tests to be sure that clangd, as a package, works as expected. 
I just need to make sure that the binary starts, that I can send some data and 
I am getting some outputs which makes sense.
https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/10/debian/qualify-clang.sh#L106-266
 (I know it is ugly :)

With my Mozilla hat, I was interested to see how clangd works and we could call 
it from tooling. I think this doc helps understanding how it works quickly. 
Without spending too much time reading LSP or reading at the code of extensions.

About the -lit-test thing, it is because I am a noob :) I would be interested 
to know if there is a better way!

Happy to do any change you want, just let me know!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75985



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

[PATCH] D75997: [ARM,MVE] Fix user-namespace violation in arm_mve.h.

[Mikhail Maltsev via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75997



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

[PATCH] D75903: [AArch64][CodeGen] Fixing stack alignment of HFA arguments on AArch64 PCS

[Lucas Prates via Phabricator via cfe-commits]

pratlucas added a comment.

I believe rule B.2 from the AAPCS64 should take precedence over rule B.5 for 
HFA arguments:

  B.2   If the argument type is an HFA or an HVA, then the argument is used 
unmodified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75903



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

[PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

To avoid bot breakage, I would recommend testing -DLLVM_ENABLE_MODULES=1 stage2 
build works before landing this, as it is notoriously sensitive to header 
reshuffling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75784



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

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

Szelethus added a comment.

Could you please add the warning we discussed? That would be a great 
demonstration of this patch's capabilities, and wouldn't deserve its own patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682



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

[PATCH] D75360: [analyzer][NFC] Tie CheckerRegistry to CheckerManager, allow CheckerManager to be constructed for non-analysis purposes

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

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



Comment at: clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:217
+mgr.template registerChecker();
   }
 

baloghadamsoftware wrote:
> Why do we need `MGR` here as template parameter? Is this function also used 
> for other class instances than `CheckerManager`? I fail to see such 
> invocation.
Include cycles :) `CheckerManager.h` includes `CheckerRegistry.h`, so we can 
only forward declare `CheckerManager`, hence the need for a template parameter.


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

https://reviews.llvm.org/D75360



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

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

Szelethus added a comment.

So, as far as I understand, your thinking here is that `CXXOperatorCallExpr`s 
(which are global, non-member operators) when they are `>>`, they will 
propagate taintedness from the 0th argument to the 1st and the return value, 
correct? So, if I do this:

  struct Panda {};
  
  // Turn integers into pandas!
  bool operator>>(int i, Panda p) {
return printf("Panda integer party!");
  }
  
  // Turn characters into pandas!
  bool operator>>(char i, Panda p) {
return printf("Panda character party!");
  }
  
  int main() {
Panda p1, p2;
bool success1 = getTaintedInt() >> p1;
bool success2 = getTaintedChar() >> p2;
// success1, success2, p1, p2 are all tainted
  }

Are we sure this is what we want? If this is a heuristic, we should document it 
well, and even then I'm not sure whether we want it. I'm also pretty sure this 
would make the eventual change to `CallDescriptionMap` more difficult, because 
the way taintedness is propagated around `std::basic_istream` not really the 
property of the global `>>` operator and what its parameters are, but rather 
the property of `std::basic_istream::operator>>`, right? What do 
you think?




Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:289-293
+  {"c_str", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"data", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"size", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"length", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"getline", {"std::", {{0}, {1, ReturnValueIndex};

Hmm, is this the appropriate place to put these? It seems like this job is 
handled in `getTaintPropagationRule`. I thought `CustomPropagations` are 
reserved for the config file.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:579
+ CheckerContext &C) {
+  const auto *OCE = dyn_cast(Call.getOriginExpr());
+  if (OCE) {

As far as I know, `Call.getOriginExpr()` may be null, we should probably use 
`dyn_cast_or_null`.




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

https://reviews.llvm.org/D71524



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

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D59214#1917149 , @lebedev.ri wrote:

> In D59214#1916596 , @sammccall wrote:
>
> > So if I understand the history here:
> >
> > - the `IsOMPStructuredBlock` bit on `Stmt` was added to implement 
> > `getStructuredBlock()`
> > - after review, `getStructuredBlock()` doesn't use the bit
> > - the bit is used in the textual AST dump, and an AST matcher (which is in 
> > turn never used in-tree)
> >
> >   Can we have the bit back please :-) We're currently trying to add a 
> > HasErrors bit to improve error-recovery and some nodes are completely full. 
> > Using a bit on every Stmt to improve the text dump a little and provide a 
> > matcher (only relevant to OMP files) seems disproportionately expensive.
>
>
>
>
> In D59214#1917106 , @rjmccall wrote:
>
> > If the bit is unused except for propagation, please remove it.
>
>
> This review (& https://bugs.llvm.org/show_bug.cgi?id=40563 in general)
>  has left me so burnout as to all this openmp stuff that i never
>  followed-up with the actual planned usages of this bit.


I'm sorry to hear that.

> The reasons why this bit is needed (i think they were spelled out here)
>  still apply. One obvious example is 
> https://bugs.llvm.org/show_bug.cgi?id=41102,
>  where after we'd teach `ExceptionAnalyzer` to look into `CapturedStmt` / 
> `CapturedDecl`,
>  we'd immediately have false-positives for these OpenMP structured blocks,
>  so we'd need to avoid traversing into them.
> 
> Doing that via a single bit check should be trivial, as opposed to,
>  i dunno, maintainting a set of all the openmp structured block statements,
>  and before visiting each statement seeing if it is in that set?
>  That kinda sounds slow.

Well, two points.

The first is that language dialects have to take a back-seat to general-purpose
mechanisms.  I care a lot about Objective-C ARC, and that dialect has some
type qualifiers that are used very frequently, but the representation does not
and will never prioritize those type qualifiers over things that are common to
C/C++, and so we just use extra memory when compiling ARC.

The second is that we can pretty easily solve your problem without adding a
bit to `Stmt`.  Captured statements are like blocks in the sense that traverses
that walk into captured statements will always pass through a node in the AST
that knows it's introducing a captured statement, so they should be easy to just
record that the traversal is currently within a captured statement, or even
maintain a stack of such contexts.  If it helps to add "callbacks" to some CRTP
visitor class when entering and exiting a special local context, we can do that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59214



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

[PATCH] D75786: [clang-tidy] Move fuchsia-restrict-system-includes to portability module for general use.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast added a comment.

In D75786#1916714 , @aaron.ballman 
wrote:

> In D75786#1916637 , @RKSimon wrote:
>
> > @PaulkaToast This patch appears to have caused a buildbot issue, please can 
> > you investigate/revert: 
> > http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/63869
>
>
> I believe the issue is that we're looking for system headers that are not 
> mocked as part of the test. Sorry about not catching that earlier in the 
> review! @PaulkaToast, you should create an empty `stdio.h` (and others used 
> by your test) in Inputs/fucscia-restrict-system-includes/system so that we're 
> not finding the actual system includes.


Working on that now, I'll send out a review shortly. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75786



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

[PATCH] D73231: [CUDA] Assume the latest known CUDA version if we've found an unknown one.

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:51
+  // the latest version we support.
+  D.Diag(diag::warn_drv_unknown_cuda_version)
+  << MajorMinor << CudaVersionToString(CudaVersion::LATEST);

We got some issue with this warning.

Basically CudaInstallationDetector is a member of GNU and other host toolchain 
and this check is done in the ctor.

If a user has latest CUDA SDK installed, clang will always emit a warning. If 
the user has -Werror set, which is quite common, compilation will always fail.

Is it possible only do this check for CUDA compilation?

Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73231



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

[PATCH] D74973: [analyzer] StdLibraryFunctionsChecker refactor w/ inheritance

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

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:102
+  /// Given a range, should the argument stay inside or outside this range?
+  enum RangeKind { OutOfRange, WithinRange };
 

I would move this into the class to encapsulate the values instead of 
contaminating namespace `ento`.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:151
+
+  using ValueConstraintPtr = std::shared_ptr;
+  /// The complete list of constraints that defines a single branch.

Szelethus wrote:
> martong wrote:
> > Szelethus wrote:
> > > martong wrote:
> > > > martong wrote:
> > > > > Szelethus wrote:
> > > > > > gamesh411 wrote:
> > > > > > > martong wrote:
> > > > > > > > Note here, we need a copyable, polymorphic and default 
> > > > > > > > initializable type (vector needs that). A raw pointer were 
> > > > > > > > good, however, we cannot default initialize that. unique_ptr 
> > > > > > > > makes the Summary class non-copyable, therefore not an option.
> > > > > > > > Releasing the copyablitly requirement would render the 
> > > > > > > > initialization of the Summary map infeasible.
> > > > > > > > Perhaps we could come up with a [[ 
> > > > > > > > https://www.youtube.com/watch?v=bIhUE5uUFOA | type erasure 
> > > > > > > > technique without inheritance ]] once we consider the 
> > > > > > > > shared_ptr as restriction, but for now that seems to be 
> > > > > > > > overkill.
> > > > > > > std::variant (with std::monostate for the default 
> > > > > > > constructibility) would also be an option  (if c++17 were 
> > > > > > > supported). But this is not really an issue, i agree with that.
> > > > > > Ugh, we've historically been very hostile towards virtual 
> > > > > > functions. We don't mind them that much when they don't have to run 
> > > > > > a lot, like during bug report construction, but as a core part of 
> > > > > > the analysis, I'm not sure what the current stance is on it.
> > > > > > 
> > > > > > I'm not inherently (haha) against it, and I'm fine with leaving 
> > > > > > this as-is for the time being, though I'd prefer if you placed a 
> > > > > > `TODO` to revisit this issue.
> > > > > > std::variant (with std::monostate for the default constructibility) 
> > > > > > would also be an option (if c++17 were supported). But this is not 
> > > > > > really an issue, i agree with that.
> > > > > 
> > > > > Variant would be useful if we knew the set of classes prior and we 
> > > > > wanted to add operations gradually. Class hierarchies (or run-time 
> > > > > concepts [Sean Parent]) are very useful if we know the set of 
> > > > > operations prior and we want to add classes gradually, and we have 
> > > > > this case here.
> > > > > Ugh, we've historically been very hostile towards virtual functions. 
> > > > > We don't mind them that much when they don't have to run a lot, like 
> > > > > during bug report construction, but as a core part of the analysis, 
> > > > > I'm not sure what the current stance is on it.
> > > > 
> > > > I did not find any evidence for this statement. Consider as a counter 
> > > > example the ExternalASTSource interface in Clang, which is filled with 
> > > > virtual functions and is part of the C/C++ lookup mechanism, which is 
> > > > quite on the hot path of C/C++ parsing I think. Did not find any 
> > > > prohibition in LLVM coding guidelines neither. I do believe virtual 
> > > > functions have their use cases exactly where (runtime) polimorphism is 
> > > > needed, such as in this patch.
> > > > 
> > > I consider myself proven wrong here then.
> > Thanks for the review and for considering other alternatives! And please 
> > accept my apologies, maybe I was pushing too hard on inheritance.
> We should definitely decorate this with a `TODO: Can we change this to not 
> use a shared_ptr?`. Worst case scenario, there it will stay for eternity :)
`FIXME` is the official, not `TODO`, afaik.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:151
+
+  using ValueConstraintPtr = std::shared_ptr;
+  /// The complete list of constraints that defines a single branch.

baloghadamsoftware wrote:
> Szelethus wrote:
> > martong wrote:
> > > Szelethus wrote:
> > > > martong wrote:
> > > > > martong wrote:
> > > > > > Szelethus wrote:
> > > > > > > gamesh411 wrote:
> > > > > > > > martong wrote:
> > > > > > > > > Note here, we need a copyable, polymorphic and default 
> > > > > > > > > initializable type (vector needs that). A raw pointer were 
> > > > > > > > > good, however, we cannot default initialize that. unique_ptr 
> > > > > > > > > makes the Summary class non-copyable, therefore not an option.
> > > > > > > > > Releasing the copyablitly requirement would render the 
> > > > > > > > > initialization of the Summary map infeasible.
> > > > > > > > > Perhaps we could come 

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Looking at the issue now, it should be a straightforward fix. Will send a patch 
ASAP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



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