[PATCH] D67949: [clang-format] [PR36858] Add missing .hh and .cs extensions from python support utilities

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: pseyfert, klimek, owenpan.
MyDeveloperDay added a project: clang-tools-extra.
Herald added a project: clang.

https://bugs.llvm.org/show_bug.cgi?id=36858 identifies .hh as a missing C++ 
header extension file while making this change I realized there was no support 
for .cs files which were added recently


Repository:
  rC Clang

https://reviews.llvm.org/D67949

Files:
  clang/tools/clang-format/clang-format-diff.py
  clang/tools/clang-format/git-clang-format


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -77,13 +77,14 @@
   'c', 'h',  # C
   'm',  # ObjC
   'mm',  # ObjC++
-  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hpp', 'hxx',  # C++
+  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hh', 'hpp', 'hxx',  # C++
   'cu',  # CUDA
   # Other languages that clang-format supports
   'proto', 'protodevel',  # Protocol Buffers
   'java',  # Java
   'js',  # JavaScript
   'ts',  # TypeScript
+  'cs',  # C Sharp
   ])
 
   p = argparse.ArgumentParser(
Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -43,8 +43,8 @@
   help='custom pattern selecting file paths to reformat '
   '(case sensitive, overrides -iregex)')
   parser.add_argument('-iregex', metavar='PATTERN', default=
-  r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hpp|m|mm|inc|js|ts|proto'
-  r'|protodevel|java)',
+  
r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hh|hpp|m|mm|inc|js|ts|proto'
+  r'|protodevel|java|cs)',
   help='custom pattern selecting file paths to reformat '
   '(case insensitive, overridden by -regex)')
   parser.add_argument('-sort-includes', action='store_true', default=False,


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -77,13 +77,14 @@
   'c', 'h',  # C
   'm',  # ObjC
   'mm',  # ObjC++
-  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hpp', 'hxx',  # C++
+  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hh', 'hpp', 'hxx',  # C++
   'cu',  # CUDA
   # Other languages that clang-format supports
   'proto', 'protodevel',  # Protocol Buffers
   'java',  # Java
   'js',  # JavaScript
   'ts',  # TypeScript
+  'cs',  # C Sharp
   ])
 
   p = argparse.ArgumentParser(
Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -43,8 +43,8 @@
   help='custom pattern selecting file paths to reformat '
   '(case sensitive, overrides -iregex)')
   parser.add_argument('-iregex', metavar='PATTERN', default=
-  r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hpp|m|mm|inc|js|ts|proto'
-  r'|protodevel|java)',
+  r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hh|hpp|m|mm|inc|js|ts|proto'
+  r'|protodevel|java|cs)',
   help='custom pattern selecting file paths to reformat '
   '(case insensitive, overridden by -regex)')
   parser.add_argument('-sort-includes', action='store_true', default=False,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

In D63932#1679910 , @ostannard wrote:

> > This makes the IR self-contained, which is good, but it also make the 
> > interpretation of the IR modal, which isn't great. It means that suddenly 
> > the rules of interpretation of what is valid to do or not changes according 
> > to this module flag.
>
> I think the original version was better from that perspective - most compiler 
> passes only need to check for one value of the attribute, and only the linker 
> needed to care about the difference between link-unit and public visibility.


The *linker* needed to care about the difference is fine, but that's different 
from a *compiler pass* during LTO. In general it is an important design point 
in LLVM so far to encode these information in the IR (for instance the linkage 
type of globals would be changed and not the interpretation of these with a 
flag).

> Do you think we should go back to that design, or do you have a different 
> idea?

I didn't review the original version, but pcc@ did apparently, so I'm willing 
to trust them on what's the best way forward here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[clang-tools-extra] r372693 - [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

[Adam Balogh via cfe-commits]

Author: baloghadamsoftware
Date: Tue Sep 24 00:43:26 2019
New Revision: 372693

URL: http://llvm.org/viewvc/llvm-project?rev=372693&view=rev
Log:
[clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite 
loops

Finding infinite loops is well-known to be impossible (halting problem).
However, it is possible to detect some obvious infinite loops, for example,
if the loop condition is not changed. Detecting such loops is beneficial
since the tests will hang on programs containing infinite loops so
testing-time detection may be costly in large systems. Obvious cases are
where the programmer forgets to increment/decrement the counter or
increments/decrements the wrong variable.

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


Modified:
clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp?rev=372693&r1=372692&r2=372693&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp Tue Sep 
24 00:43:26 2019
@@ -23,6 +23,7 @@
 #include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
+#include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
@@ -88,6 +89,8 @@ public:
 "bugprone-inaccurate-erase");
 CheckFactories.registerCheck(
 "bugprone-incorrect-roundings");
+CheckFactories.registerCheck(
+"bugprone-infinite-loop");
 CheckFactories.registerCheck(
 "bugprone-integer-division");
 CheckFactories.registerCheck(

Modified: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt?rev=372693&r1=372692&r2=372693&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt Tue Sep 24 
00:43:26 2019
@@ -15,6 +15,7 @@ add_clang_library(clangTidyBugproneModul
   ForwardingReferenceOverloadCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
+  InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=372693&r1=372692&r2=372693&view=diff
==
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Tue Sep 24 00:43:26 2019
@@ -70,6 +70,86 @@ Improvements to clang-tidy
 - New :doc:`bugprone-dynamic-static-initializers
   ` check.
 
+- New OpenMP module.
+
+  For checks specific to `OpenMP `_ API.
+
+- New :doc:`abseil-duration-addition
+  ` check.
+
+  Checks for cases where addition should be performed in the ``absl::Time``
+  domain.
+
+- New :doc:`abseil-duration-conversion-cast
+  ` check.
+
+  Checks for casts of ``absl::Duration`` conversion functions, and recommends
+  the right conversion function instead.
+
+- New :doc:`abseil-duration-unnecessary-conversion
+  ` check.
+
+  Finds and fixes cases where ``absl::Duration`` values are being converted to
+  numeric types and back again.
+
+- New :doc:`abseil-time-comparison
+  ` check.
+
+  Prefer comparisons in the ``absl::Time`` domain instead of the integer
+  domain.
+
+- New :doc:`abseil-time-subtraction
+  ` check.
+
+  Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
+  in the Time domain instead of the numeric domain.
+
+- New :doc:`android-cloexec-pipe
+  ` check.
+
+  This check detects usage of ``pipe()``.
+
+- New :doc:`android-cloexec-pipe2
+  ` check.
+
+  This checks ensures that ``pipe2()`` is called with the O_CLOEXEC flag.
+
+- New :doc:`bugprone-infinite-loop
+  ` check.
+
+  Finds obvious infinite loops (loops where the condition variable is not
+  changed at all).
+
+- New :doc:`bugprone-unhandled-self-assignment
+  ` check.
+
+  Finds user-defined copy assignment operators which do not protect the code
+  against self-assignment either by checking self-assignment explicitly or
+  using the copy-and-swap or the copy-and-move method.
+
+- New :doc:`bugprone-branch-clone
+  ` check.
+
+  Checks for repeated branches in ``if/else if/else`` 

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

[Wenju He via Phabricator via cfe-commits]

wenju added a comment.

We see error "clang (LLVM option parsing): for the   --pgo-warn-misexpect 
option: may only occur zero or one times!" when 
clang::CompilerInvocation::CreateFromArgs and clang::ExecuteCompilerInvocation 
are called twice.

This line: if (Diags.isIgnored(diag::warn_profile_data_misexpect, 
SourceLocation()))
I think it should be !Diags.isIgnored.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



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

[PATCH] D64736: [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

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

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL372693: [clang-tidy] New bugprone-infinite-loop check for 
detecting obvious infinite… (authored by baloghadamsoftware, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64736?vs=221300&id=221477#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64736

Files:
  clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Index: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -23,6 +23,7 @@
 #include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
+#include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
@@ -88,6 +89,8 @@
 "bugprone-inaccurate-erase");
 CheckFactories.registerCheck(
 "bugprone-incorrect-roundings");
+CheckFactories.registerCheck(
+"bugprone-infinite-loop");
 CheckFactories.registerCheck(
 "bugprone-integer-division");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
@@ -15,6 +15,7 @@
   ForwardingReferenceOverloadCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
+  InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
Index: clang-tools-extra/trunk/docs/ReleaseNotes.rst
===
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst
@@ -70,6 +70,86 @@
 - New :doc:`bugprone-dynamic-static-initializers
   ` check.
 
+- New OpenMP module.
+
+  For checks specific to `OpenMP `_ API.
+
+- New :doc:`abseil-duration-addition
+  ` check.
+
+  Checks for cases where addition should be performed in the ``absl::Time``
+  domain.
+
+- New :doc:`abseil-duration-conversion-cast
+  ` check.
+
+  Checks for casts of ``absl::Duration`` conversion functions, and recommends
+  the right conversion function instead.
+
+- New :doc:`abseil-duration-unnecessary-conversion
+  ` check.
+
+  Finds and fixes cases where ``absl::Duration`` values are being converted to
+  numeric types and back again.
+
+- New :doc:`abseil-time-comparison
+  ` check.
+
+  Prefer comparisons in the ``absl::Time`` domain instead of the integer
+  domain.
+
+- New :doc:`abseil-time-subtraction
+  ` check.
+
+  Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
+  in the Time domain instead of the numeric domain.
+
+- New :doc:`android-cloexec-pipe
+  ` check.
+
+  This check detects usage of ``pipe()``.
+
+- New :doc:`android-cloexec-pipe2
+  ` check.
+
+  This checks ensures that ``pipe2()`` is called with the O_CLOEXEC flag.
+
+- New :doc:`bugprone-infinite-loop
+  ` check.
+
+  Finds obvious infinite loops (loops where the condition variable is not
+  changed at all).
+
+- New :doc:`bugprone-unhandled-self-assignment
+  ` check.
+
+  Finds user-defined copy assignment operators which do not protect the code
+  against self-assignment either by checking self-assignment explicitly or
+  using the copy-and-swap or the copy-and-move method.
+
+- New :doc:`bugprone-branch-clone
+  ` check.
+
+  Checks for repeated branches in ``if/else if/else`` chains, consecutive
+  repeated branches in ``switch`` statements and indentical true and false
+  branches in conditional operators.
+
+- New :doc:`bugprone-posix-return
+  ` check.
+
+  Checks if any calls to POSIX functions (except ``posix_openpt``) expect negative
+  return values.
+
+- New :doc:`fuchsia-default-arguments-calls
+  ` check.
+
+  Warns if a function or method is called with default arguments.
+  This was previously done by `fuchsia-default-arguments check`, which has been
+  removed.
+
+- New :doc:`fuchsia-default-arguments-calls
+  ` check.
+
   Finds instances where variables with static storage are initialized
   dynamically in header files.
 
@@ -103,6 +183,10 @@
   Now also checks if any calls to ``pthread_*`` functions expect negative return
   values.
 
+- New :doc:`bugprone-infinite-loop `
+  ch

[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Kadir Cetinkaya via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/FindTarget.cpp:376
+
+  // We prefer to return template instantiation, but fallback to template
+  // pattern if instantiation is not available.

ilya-biryukov wrote:
> kadircet wrote:
> > can we simplify this by populating two vectors, Instantiations and 
> > Patterns, and then returning the patterns iff instantiations are empty?
> Did something very similar. PTAL.
LG, thanks!



Comment at: clang-tools-extra/clangd/FindTarget.cpp:432
+  Ref = ReferenceLoc{
+  E->getQualifierLoc(), E->getNameInfo().getLoc(), {E->getDecl()}};
+}

ilya-biryukov wrote:
> kadircet wrote:
> > I believe it is better to return `getFoundDecl` then `getDecl`, the former 
> > respects using declarations.
> Good point. Done. Added a test too.
I was actually referring to `DeclRefExpr`, change seems to be on `MemberExpr`



Comment at: clang-tools-extra/clangd/FindTarget.cpp:548
+  bool TraverseElaboratedTypeLoc(ElaboratedTypeLoc L) {
+// ElaboratedTypeLoc will reports information for its inner type loc.
+// Otherwise we loose information about inner types loc's qualifier.

ilya-biryukov wrote:
> kadircet wrote:
> > why not just traversenestednamespecifier and `visitNode(L)` instead of 
> > calling the base::traverse ?
> To avoid re-implementing the traversal logic. `Base::Traverse` does exactly 
> that, we shouldn't re-implement traversal ourselves.
I see but that should help get rid of `TypeLocsToSkip` and some extra 
traversals. I believe it would be worth getting rid of the additional state 
management, but up to you.



Comment at: clang-tools-extra/clangd/FindTarget.cpp:370
+namespace {
+/// Find a declaration explicitly referenced in the source code defined by \p 
N.
+/// For templates, will prefer to return a template instantiation whenever

nit: s/a declaration/declaration(s)/



Comment at: clang-tools-extra/clangd/FindTarget.cpp:519
+
+namespace {
+

unnecessary closing and opening of anon namespace



Comment at: clang-tools-extra/clangd/FindTarget.cpp:577
+  /// (!) For the purposes of this function declarations are not considered to
+  /// be references. However, declarations can have:wa references inside
+  /// them, e.g. 'namespace foo = std' references namespace 'std' and this

`:wa` is still around `have:wa references` :D



Comment at: clang-tools-extra/clangd/FindTarget.cpp:609
+// FIXME: should this be done by 'explicitReference'?
+if (Ref->NameLoc.isInvalid() || Ref->NameLoc.isMacroID())
+  return;

I can see the second check is for getting rid of references coming from macro 
expansions. But what exactly is the first one for? How can we get an invalid 
nameloc, could you also add it as a comment?



Comment at: clang-tools-extra/clangd/FindTarget.h:96
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ReferenceLoc R);
+

Is this for testing purposes? maybe move it into `findtargettests.cpp` or make 
it a member helper like `Print(SourceManager&)` so that it can also print 
locations etc.?



Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:498
+std::string AnnotatedCode;
+unsigned NextOffset = 0;
+for (unsigned I = 0; I < Refs.size(); ++I) {

this sounds more like `LastOffset` or `PrevOffset`



Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:504
+  assert(Pos.isValid());
+  if (Pos.isMacroID()) // FIXME: figure out how to show macro locations.
+Pos = SM.getExpansionLoc(Pos);

I believe these are already filtered-out by `findExplicitReferences` ?



Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:600
+   "5: targets = {a::b::S}\n"
+   "6: targets = {a::b::S::type}, qualifier = 'struct S::'\n"},
+  // Simple templates.

Is it OK for this case to be different than `X::a` above?

also shouldn't this be `a::b::struct S` or `None` ?(my preference would be 
towards None)



Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:638
+   "0: targets = {y}\n"
+   "1: targets = {Y::foo}\n"},
+  };

again qualifiers seems to be inconsistent with the rest of the cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826



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

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/Macro.h:59
+
+if (auto Range = getTokenRange(SM, LangOpts, MacroNameTok.getLocation())) {
+  MainFileMacros.push_back(

hokein wrote:
> ilya-biryukov wrote:
> > Converting here is too early, could we keep this conversion in syntax 
> > highlighting code?
> > Keeping source locations here is enough.
> I'm not sure this is doable -- to get a range, we need the `SourceManager`, 
> in the syntax highlighting context, we get the SourceManager from the 
> `ParsedAST`, this `SourceManager` is used when building the main AST with 
> preamble, I don't think we can use this  `SourceManager` to get the token 
> range for macros in the preamble section?
Ah, yeah, good point. The source manager from preamble is not there anymore.
Ranges LG and this is what we used before.

Thanks for clearing this up.



Comment at: clang-tools-extra/clangd/Macro.h:67
+  bool InMainFile = true;
+  std::vector &MainFileMacros;
+};

hokein wrote:
> ilya-biryukov wrote:
> > Could we model in a way that avoids duplicating macro names on each 
> > occurrence?
> > We had `StringSet Names` and `vector Locations`, let's keep 
> > it in the same way.
> > 
> > We could group this into a struct to reduce boilerplate of transferring it 
> > around, obviously
> > ```
> > struct MainFileMacros {
> >   StringSet Names;
> >   vector Locations;
> > };
> > ```
> yes, we don't need the name and location reletionship in this patch, but we'd 
> need this when implementing xrefs for macros. do you think we should keep the 
> same way, and do the change when we start implementing xrefs for macros?
Cross-references have to be more fine-grained and having only a name is not 
enough. Let's address this separately when/if we actually need this in xrefs.
In particular, we want to handle macro re-definitions:
```
#define FOO 123 // #1
int a = FOO; // a reference to #1

#define FOO 234 // #2
int b = FOO; // a reference to #2
```
Current model will merge these two together as both names are the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67496



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

[clang-tools-extra] r372704 - Revert rL372693 : [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

[Simon Pilgrim via cfe-commits]

Author: rksimon
Date: Tue Sep 24 01:56:44 2019
New Revision: 372704

URL: http://llvm.org/viewvc/llvm-project?rev=372704&view=rev
Log:
Revert rL372693 : [clang-tidy] New bugprone-infinite-loop check for detecting 
obvious infinite loops

Finding infinite loops is well-known to be impossible (halting problem).
However, it is possible to detect some obvious infinite loops, for example,
if the loop condition is not changed. Detecting such loops is beneficial
since the tests will hang on programs containing infinite loops so
testing-time detection may be costly in large systems. Obvious cases are
where the programmer forgets to increment/decrement the counter or
increments/decrements the wrong variable.

Differential Revision: https://reviews.llvm.org/D64736
---
Broke some buildbots "No SOURCES given to target: obj.clangTidyBugproneModule"

Modified:
clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp?rev=372704&r1=372703&r2=372704&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp Tue Sep 
24 01:56:44 2019
@@ -23,7 +23,6 @@
 #include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
-#include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
@@ -89,8 +88,6 @@ public:
 "bugprone-inaccurate-erase");
 CheckFactories.registerCheck(
 "bugprone-incorrect-roundings");
-CheckFactories.registerCheck(
-"bugprone-infinite-loop");
 CheckFactories.registerCheck(
 "bugprone-integer-division");
 CheckFactories.registerCheck(

Modified: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt?rev=372704&r1=372703&r2=372704&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt Tue Sep 24 
01:56:44 2019
@@ -15,7 +15,6 @@ add_clang_library(clangTidyBugproneModul
   ForwardingReferenceOverloadCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
-  InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=372704&r1=372703&r2=372704&view=diff
==
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Tue Sep 24 01:56:44 2019
@@ -70,86 +70,6 @@ Improvements to clang-tidy
 - New :doc:`bugprone-dynamic-static-initializers
   ` check.
 
-- New OpenMP module.
-
-  For checks specific to `OpenMP `_ API.
-
-- New :doc:`abseil-duration-addition
-  ` check.
-
-  Checks for cases where addition should be performed in the ``absl::Time``
-  domain.
-
-- New :doc:`abseil-duration-conversion-cast
-  ` check.
-
-  Checks for casts of ``absl::Duration`` conversion functions, and recommends
-  the right conversion function instead.
-
-- New :doc:`abseil-duration-unnecessary-conversion
-  ` check.
-
-  Finds and fixes cases where ``absl::Duration`` values are being converted to
-  numeric types and back again.
-
-- New :doc:`abseil-time-comparison
-  ` check.
-
-  Prefer comparisons in the ``absl::Time`` domain instead of the integer
-  domain.
-
-- New :doc:`abseil-time-subtraction
-  ` check.
-
-  Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
-  in the Time domain instead of the numeric domain.
-
-- New :doc:`android-cloexec-pipe
-  ` check.
-
-  This check detects usage of ``pipe()``.
-
-- New :doc:`android-cloexec-pipe2
-  ` check.
-
-  This checks ensures that ``pipe2()`` is called with the O_CLOEXEC flag.
-
-- New :doc:`bugprone-infinite-loop
-  ` check.
-
-  Finds obvious infinite loops (loops where the condition variable is not
-  changed at all).
-
-- New :doc:`bugprone-unhandled-self-assignment
-  ` check.
-
-  Finds user-defined copy assignment operators which do not protect the code
-  against self-assignment either by checking self-assignment explicitly or
-  using the copy-and-swap or the copy-and-move method.
-
-- New :doc

Re: [clang-tools-extra] r372693 - [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

[Mikael Holmén via cfe-commits]

Hi Adam,

Is InfiniteLoopCheck.cpp missing?

I get

CMake Error at /data/repo/master/llvm/cmake/modules/AddLLVM.cmake:443
(add_library):
  Cannot find source file:

InfiniteLoopCheck.cpp


/Mikael


On Tue, 2019-09-24 at 07:43 +, Adam Balogh via cfe-commits wrote:
> Author: baloghadamsoftware
> Date: Tue Sep 24 00:43:26 2019
> New Revision: 372693
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=372693&view=rev
> Log:
> [clang-tidy] New bugprone-infinite-loop check for detecting obvious
> infinite loops
> 
> Finding infinite loops is well-known to be impossible (halting
> problem).
> However, it is possible to detect some obvious infinite loops, for
> example,
> if the loop condition is not changed. Detecting such loops is
> beneficial
> since the tests will hang on programs containing infinite loops so
> testing-time detection may be costly in large systems. Obvious cases
> are
> where the programmer forgets to increment/decrement the counter or
> increments/decrements the wrong variable.
> 
> Differential Revision: https://reviews.llvm.org/D64736
> 
> 
> Modified:
> clang-tools-extra/trunk/clang-
> tidy/bugprone/BugproneTidyModule.cpp
> clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
> clang-tools-extra/trunk/docs/ReleaseNotes.rst
> clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
> 
> Modified: clang-tools-extra/trunk/clang-
> tidy/bugprone/BugproneTidyModule.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp?rev=372693&r1=372692&r2=372693&view=diff
> =
> =
> --- clang-tools-extra/trunk/clang-
> tidy/bugprone/BugproneTidyModule.cpp (original)
> +++ clang-tools-extra/trunk/clang-
> tidy/bugprone/BugproneTidyModule.cpp Tue Sep 24 00:43:26 2019
> @@ -23,6 +23,7 @@
>  #include "ForwardingReferenceOverloadCheck.h"
>  #include "InaccurateEraseCheck.h"
>  #include "IncorrectRoundingsCheck.h"
> +#include "InfiniteLoopCheck.h"
>  #include "IntegerDivisionCheck.h"
>  #include "LambdaFunctionNameCheck.h"
>  #include "MacroParenthesesCheck.h"
> @@ -88,6 +89,8 @@ public:
>  "bugprone-inaccurate-erase");
>  CheckFactories.registerCheck(
>  "bugprone-incorrect-roundings");
> +CheckFactories.registerCheck(
> +"bugprone-infinite-loop");
>  CheckFactories.registerCheck(
>  "bugprone-integer-division");
>  CheckFactories.registerCheck(
> 
> Modified: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt?rev=372693&r1=372692&r2=372693&view=diff
> =
> =
> --- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
> (original)
> +++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt Tue
> Sep 24 00:43:26 2019
> @@ -15,6 +15,7 @@ add_clang_library(clangTidyBugproneModul
>ForwardingReferenceOverloadCheck.cpp
>InaccurateEraseCheck.cpp
>IncorrectRoundingsCheck.cpp
> +  InfiniteLoopCheck.cpp
>IntegerDivisionCheck.cpp
>LambdaFunctionNameCheck.cpp
>MacroParenthesesCheck.cpp
> 
> Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=372693&r1=372692&r2=372693&view=diff
> =
> =
> --- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
> +++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Tue Sep 24 00:43:26
> 2019
> @@ -70,6 +70,86 @@ Improvements to clang-tidy
>  - New :doc:`bugprone-dynamic-static-initializers
>` check.
>  
> +- New OpenMP module.
> +
> +  For checks specific to `OpenMP <
> https://protect2.fireeye.com/url?k=da8eb47f-860496b0-da8ef4e4-0cc47ad93ea4-0c1ed127bf5b3ffa&q=1&u=https%3A%2F%2Fwww.openmp.org%2F>`_
>  API.
> +
> +- New :doc:`abseil-duration-addition
> +  ` check.
> +
> +  Checks for cases where addition should be performed in the
> ``absl::Time``
> +  domain.
> +
> +- New :doc:`abseil-duration-conversion-cast
> +  ` check.
> +
> +  Checks for casts of ``absl::Duration`` conversion functions, and
> recommends
> +  the right conversion function instead.
> +
> +- New :doc:`abseil-duration-unnecessary-conversion
> +  ` check.
> +
> +  Finds and fixes cases where ``absl::Duration`` values are being
> converted to
> +  numeric types and back again.
> +
> +- New :doc:`abseil-time-comparison
> +  ` check.
> +
> +  Prefer comparisons in the ``absl::Time`` domain instead of the
> integer
> +  domain.
> +
> +- New :doc:`abseil-time-subtraction
> +  ` check.
> +
> +  Finds and fixes ``absl::Time`` subtraction expressions to do
> subtraction
> +  in the Time domain instead of the numeric domain.
> +
> +- New :doc:`android-cloexec-pipe
> +  ` check.
> +
> +  This check detects usage of ``pipe()``.
>

[PATCH] D64736: [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

[Simon Pilgrim via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcb3d969453c9: Revert rL372693 : [clang-tidy] New 
bugprone-infinite-loop check for detecting… (authored by RKSimon).

Changed prior to commit:
  https://reviews.llvm.org/D64736?vs=221477&id=221484#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64736

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst

Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -51,7 +51,6 @@
bugprone-forwarding-reference-overload
bugprone-inaccurate-erase
bugprone-incorrect-roundings
-   bugprone-infinite-loop
bugprone-integer-division
bugprone-lambda-function-name
bugprone-macro-parentheses
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -70,86 +70,6 @@
 - New :doc:`bugprone-dynamic-static-initializers
   ` check.
 
-- New OpenMP module.
-
-  For checks specific to `OpenMP `_ API.
-
-- New :doc:`abseil-duration-addition
-  ` check.
-
-  Checks for cases where addition should be performed in the ``absl::Time``
-  domain.
-
-- New :doc:`abseil-duration-conversion-cast
-  ` check.
-
-  Checks for casts of ``absl::Duration`` conversion functions, and recommends
-  the right conversion function instead.
-
-- New :doc:`abseil-duration-unnecessary-conversion
-  ` check.
-
-  Finds and fixes cases where ``absl::Duration`` values are being converted to
-  numeric types and back again.
-
-- New :doc:`abseil-time-comparison
-  ` check.
-
-  Prefer comparisons in the ``absl::Time`` domain instead of the integer
-  domain.
-
-- New :doc:`abseil-time-subtraction
-  ` check.
-
-  Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
-  in the Time domain instead of the numeric domain.
-
-- New :doc:`android-cloexec-pipe
-  ` check.
-
-  This check detects usage of ``pipe()``.
-
-- New :doc:`android-cloexec-pipe2
-  ` check.
-
-  This checks ensures that ``pipe2()`` is called with the O_CLOEXEC flag.
-
-- New :doc:`bugprone-infinite-loop
-  ` check.
-
-  Finds obvious infinite loops (loops where the condition variable is not
-  changed at all).
-
-- New :doc:`bugprone-unhandled-self-assignment
-  ` check.
-
-  Finds user-defined copy assignment operators which do not protect the code
-  against self-assignment either by checking self-assignment explicitly or
-  using the copy-and-swap or the copy-and-move method.
-
-- New :doc:`bugprone-branch-clone
-  ` check.
-
-  Checks for repeated branches in ``if/else if/else`` chains, consecutive
-  repeated branches in ``switch`` statements and indentical true and false
-  branches in conditional operators.
-
-- New :doc:`bugprone-posix-return
-  ` check.
-
-  Checks if any calls to POSIX functions (except ``posix_openpt``) expect negative
-  return values.
-
-- New :doc:`fuchsia-default-arguments-calls
-  ` check.
-
-  Warns if a function or method is called with default arguments.
-  This was previously done by `fuchsia-default-arguments check`, which has been
-  removed.
-
-- New :doc:`fuchsia-default-arguments-calls
-  ` check.
-
   Finds instances where variables with static storage are initialized
   dynamically in header files.
 
@@ -183,10 +103,6 @@
   Now also checks if any calls to ``pthread_*`` functions expect negative return
   values.
 
-- New :doc:`bugprone-infinite-loop `
-  check to detect obvious infinite loops (loops where the condition variable is
-  not changed at all).
-
 Improvements to include-fixer
 -
 
Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -15,7 +15,6 @@
   ForwardingReferenceOverloadCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
-  InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -23,7 +23,6 @@
 #include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"

[PATCH] D64736: [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon reopened this revision.
RKSimon added a comment.

@baloghadamsoftware I'm sorry but I've reverted this at as rL372704 
 it was causing buildbot failures in cmake: 
http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/18147


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64736



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

r372705 - [ASTImporter] 4th attempt to fix Windows buildbot test errors

[Gabor Marton via cfe-commits]

Author: martong
Date: Tue Sep 24 02:00:46 2019
New Revision: 372705

URL: http://llvm.org/viewvc/llvm-project?rev=372705&view=rev
Log:
[ASTImporter] 4th attempt to fix Windows buildbot test errors

Modified:
cfe/trunk/unittests/AST/ASTImporterODRStrategiesTest.cpp

Modified: cfe/trunk/unittests/AST/ASTImporterODRStrategiesTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTImporterODRStrategiesTest.cpp?rev=372705&r1=372704&r2=372705&view=diff
==
--- cfe/trunk/unittests/AST/ASTImporterODRStrategiesTest.cpp (original)
+++ cfe/trunk/unittests/AST/ASTImporterODRStrategiesTest.cpp Tue Sep 24 
02:00:46 2019
@@ -580,10 +580,12 @@ ASTIMPORTER_ODR_INSTANTIATE_TYPED_TEST_C
 // Instantiate the tests.
 // ==
 
+// FIXME: These fail on Windows.
+#if !defined(_WIN32)
 INSTANTIATE_TEST_CASE_P(
 ODRViolationTests, FunctionConservative,
-// These tests fail on Windows.
-::testing::Values(ArgVector{"-target", "x86_64-pc-linux-gnu"}), );
+DefaultTestValuesForRunOptions, );
+#endif
 INSTANTIATE_TEST_CASE_P(
 ODRViolationTests, TypedefConservative,
 DefaultTestValuesForRunOptions, );
@@ -623,10 +625,12 @@ INSTANTIATE_TEST_CASE_P(
 //ODRViolationTests, VarTemplateSpecConservative,
 //DefaultTestValuesForRunOptions, );
 
+// FIXME: These fail on Windows.
+#if !defined(_WIN32)
 INSTANTIATE_TEST_CASE_P(
 ODRViolationTests, FunctionLiberal,
-// These tests fail on Windows.
-::testing::Values(ArgVector{"-target", "x86_64-pc-linux-gnu"}), );
+DefaultTestValuesForRunOptions, );
+#endif
 INSTANTIATE_TEST_CASE_P(
 ODRViolationTests, TypedefLiberal,
 DefaultTestValuesForRunOptions, );


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

[PATCH] D67932: [analyzer] Fix accidentally skipping the call during inlined defensive check suppression.

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

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

If you could change that, it would be awesome! But since this revision has its 
own side effect, let's commit as-is. LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D67932



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

[PATCH] D64736: [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

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

baloghadamsoftware added a comment.

In D64736#1680309 , @RKSimon wrote:

> @baloghadamsoftware I'm sorry but I've reverted this at as rL372704 
>  it was causing buildbot failures in 
> cmake: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/18147


Oops! Sorry! I forgot `svn add`. However we do not get e-mails about buildbot 
failures for a couple of weeks. I will retry now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64736



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

[clang-tools-extra] r372706 - [clang-tidy] Add missing InfiniteLoopCheck.h, InfiniteLoopCheck.cpp and test from D64736

[Fangrui Song via cfe-commits]

Author: maskray
Date: Tue Sep 24 02:06:31 2019
New Revision: 372706

URL: http://llvm.org/viewvc/llvm-project?rev=372706&view=rev
Log:
[clang-tidy] Add missing InfiniteLoopCheck.h, InfiniteLoopCheck.cpp and test 
from D64736

Added:
clang-tools-extra/trunk/clang-tidy/bugprone/InfiniteLoopCheck.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/InfiniteLoopCheck.h
clang-tools-extra/trunk/test/clang-tidy/bugprone-infinite-loop.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp?rev=372706&r1=372705&r2=372706&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp Tue Sep 
24 02:06:31 2019
@@ -23,6 +23,7 @@
 #include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
+#include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
@@ -88,6 +89,8 @@ public:
 "bugprone-inaccurate-erase");
 CheckFactories.registerCheck(
 "bugprone-incorrect-roundings");
+CheckFactories.registerCheck(
+"bugprone-infinite-loop");
 CheckFactories.registerCheck(
 "bugprone-integer-division");
 CheckFactories.registerCheck(

Modified: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt?rev=372706&r1=372705&r2=372706&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt Tue Sep 24 
02:06:31 2019
@@ -15,6 +15,7 @@ add_clang_library(clangTidyBugproneModul
   ForwardingReferenceOverloadCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
+  InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp

Added: clang-tools-extra/trunk/clang-tidy/bugprone/InfiniteLoopCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/InfiniteLoopCheck.cpp?rev=372706&view=auto
==
--- clang-tools-extra/trunk/clang-tidy/bugprone/InfiniteLoopCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/InfiniteLoopCheck.cpp Tue Sep 
24 02:06:31 2019
@@ -0,0 +1,189 @@
+//===--- InfiniteLoopCheck.cpp - clang-tidy 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "InfiniteLoopCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+static internal::Matcher
+loopEndingStmt(internal::Matcher Internal) {
+  return stmt(anyOf(breakStmt(Internal), returnStmt(Internal),
+gotoStmt(Internal), cxxThrowExpr(Internal),
+callExpr(Internal, callee(functionDecl(isNoReturn());
+}
+
+/// \brief Return whether `S` is a reference to the declaration of `Var`.
+static bool isAccessForVar(const Stmt *S, const VarDecl *Var) {
+  if (const auto *DRE = dyn_cast(S))
+return DRE->getDecl() == Var;
+
+  return false;
+}
+
+/// \brief Return whether `Var` has a pointer of reference in `S`.
+static bool isPtrOrReferenceForVar(const Stmt *S, const VarDecl *Var) {
+  if (const auto *DS = dyn_cast(S)) {
+for (const Decl *D : DS->getDeclGroup()) {
+  if (const auto *LeftVar = dyn_cast(D)) {
+if (LeftVar->hasInit() && LeftVar->getType()->isReferenceType()) {
+  return isAccessForVar(LeftVar->getInit(), Var);
+}
+  }
+}
+  } else if (const auto *UnOp = dyn_cast(S)) {
+if (UnOp->getOpcode() == UO_AddrOf)
+  return isAccessForVar(UnOp->getSubExpr(), Var);
+  }
+
+  return false;
+}
+
+/// \brief Return whether `Var` has a pointer of reference in `S`.
+static bool hasPtrOrReferenceInStmt(const Stmt *S, const VarDecl *Var) {
+  if (isPtrOrReferenceForVar(S, Var))
+return true;
+
+  for (const Stmt *Child 

[PATCH] D67907: [clangd] Implement a smart version of HeaderSource switch.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:21
+// file.
+class CollectIndexableLocalDecls {
+public:

kadircet wrote:
> this class doesn't seem to have any state(apart from saving AST in 
> constructor and using it in call to collect), why not just have a 
> `vector collectDecls(ParsedAST& AST);` ?
yeah, but we need to do recursive calls, which I thought making a class is more 
readable. Change to a single function with recursive lambda, I hope it won't 
hurt the code readability.



Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:61
+
+llvm::Optional resolveURI(const char *FileURI,
+   llvm::StringRef HintPath) {

kadircet wrote:
> It seems like we have a bunch of different implementations for this function 
> in background.cpp, codecomplete.cpp, ... Basically any place calling 
> `URI::resolve`, do you mind adding an overload for `URI::resolve` that'll 
> take a `const char*` instead of a `URI`?
Fair suggestion, done in r372617.



Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:78
+
+llvm::Optional getCorrespondingHeaderOrSource(const Path &OriginalFile,
+ParsedAST &AST,

kadircet wrote:
> As discussed in the design for define out-of-line, this function's dependency 
> on AST and Index should rather be optional.
> 
> - if we have AST, then we can boost the files containing the canonical 
> declarations for symbols defined in the main file.
> - if we have just the Index, currently there's nothing much we can do, but we 
> can put a fixme to add a new endpoint to symbol index for "fetching symbols 
> in a given file",
>   then we can use symbol information for declaration/definition files without 
> an AST.
> - if we have AST+Index, we can boost the files containing the definition for 
> symbols declared in the main file and vice-versa(what you already do in this 
> patch).
> - If we don't have anything we'll just make use of the filename, by changing 
> the extension, as current `ClangdServer::switchSourceHeader` implementation 
> does.
as discussed offline, we will
- make the filename heuristic into a separate API which provides more flexible;
- make this API support 3 cases (except the case where we only have index, as 
it'd require large effort to implement "fetching symbols in a given file" in 
the index, we're less certain it is worth); when we only have AST (or the index 
doesn't contain any interesting information), we just use the information for 
the AST to do the ".cc=>.h" inference.

  



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67907



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

[PATCH] D67907: [clangd] Implement a smart version of HeaderSource switch.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 221486.
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/D67907/new/

https://reviews.llvm.org/D67907

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/HeaderSourceSwitch.cpp
  clang-tools-extra/clangd/HeaderSourceSwitch.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp

Index: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
@@ -0,0 +1,174 @@
+//===--- HeaderSourceSwitchTests.cpp - ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "HeaderSourceSwitch.h"
+
+#include "TestFS.h"
+#include "TestTU.h"
+#include "index/MemIndex.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+MATCHER_P(DeclNamed, Name, "") {
+  if (const NamedDecl *ND = dyn_cast(arg))
+if (ND->getQualifiedNameAsString() == Name)
+  return true;
+  return false;
+}
+
+TEST(HeaderSourceSwitchTest, GetLocalDecls) {
+  TestTU TU;
+  TU.HeaderCode = R"cpp(
+  void HeaderOnly();
+  )cpp";
+  TU.Code = R"cpp(
+  void MainF1();
+  class Foo {};
+  namespace ns {
+  class Foo {
+void method();
+int field;
+  };
+  } // namespace ns
+
+  // Non-indexable symbols
+  namespace {
+  void Ignore1() {}
+  }
+
+  )cpp";
+
+  auto AST = TU.build();
+  EXPECT_THAT(getIndexableLocalDecls(AST),
+  testing::UnorderedElementsAre(
+  DeclNamed("MainF1"), DeclNamed("Foo"), DeclNamed("ns::Foo"),
+  DeclNamed("ns::Foo::method"), DeclNamed("ns::Foo::field")));
+}
+
+TEST(HeaderSourceSwitchTest, FromHeaderToSource) {
+  // build a proper index, which contains symbols:
+  //   A_Sym1, declared in TestTU.h, defined in a.cpp
+  //   B_Sym[1-2], declared in TestTU.h, defined in b.cpp
+  SymbolSlab::Builder AllSymbols;
+  TestTU Testing;
+  Testing.HeaderFilename = "TestTU.h";
+  Testing.HeaderCode = "void A_Sym1();";
+  Testing.Filename = "a.cpp";
+  Testing.Code = "void A_Sym1() {};";
+  for (auto &Sym : Testing.headerSymbols())
+AllSymbols.insert(Sym);
+
+  Testing.HeaderCode = R"cpp(
+  void B_Sym1();
+  void B_Sym2();
+  )cpp";
+  Testing.Filename = "b.cpp";
+  Testing.Code = R"cpp(
+  void B_Sym1() {}
+  void B_Sym2() {}
+  )cpp";
+  for (auto &Sym : Testing.headerSymbols())
+AllSymbols.insert(Sym);
+  auto Index = MemIndex::build(std::move(AllSymbols).build(), {}, {});
+
+  // Test for swtich from .h header to .cc source
+  struct {
+llvm::StringRef HeaderCode;
+llvm::Optional ExpectedSource;
+  } TestCases[] = {{R"cpp(// empty, no header found)cpp", llvm::None},
+   {R"cpp(
+   // no definition found in the index.
+   void NonDefinition();
+)cpp",
+llvm::None},
+   {R"cpp(
+   void A_Sym1();
+  )cpp",
+testPath("a.cpp")},
+   {R"cpp(
+   // b.cpp wins.
+   void A_Sym1();
+   void B_Sym1();
+   void B_Sym2();
+)cpp",
+testPath("b.cpp")}};
+  const std::string &TestFilePath = testPath("TestTU.h");
+  for (const auto &Case : TestCases) {
+TestTU TU = TestTU::withCode(Case.HeaderCode);
+TU.ExtraArgs.push_back("-xc++-header"); // inform clang this is a header.
+auto HeaderAST = TU.build();
+EXPECT_EQ(Case.ExpectedSource, getCorrespondingHeaderOrSource(
+   TestFilePath, HeaderAST, Index.get()));
+  }
+}
+
+TEST(HeaderSourceSwitchTest, FromSourceToHeader) {
+  // build a proper index, which contains symbols:
+  //   A_Sym1, declared in a.h, defined in TestTU.cpp
+  //   B_Sym[1-2], declared in b.h, defined in TestTU.cpp
+  TestTU TUForIndex = TestTU::withCode(R"cpp(
+  #include "a.h"
+  #include "b.h"
+
+  void A_Sym1() {}
+
+  void B_Sym1() {}
+  void B_Sym2() {}
+  )cpp");
+  TUForIndex.AdditionalFiles["a.h"] = R"cpp(
+  void A_Sym1();
+  )cpp";
+  TUForIndex.AdditionalFiles["b.h"] = R"cpp(
+  void B_Sym1();
+  void B_Sym2();
+  )cpp";
+  TUForIndex.Filename = "TestTU.cpp";
+  auto Index = TUForIndex.index();
+
+  // Test for switching from .cc source file to .h header.
+  struct {
+llvm::StringRef SourceCode;
+llvm::Optional ExpectedResult;
+  } TestCases[] = {
+  {R"cpp(// empty, no header found)cpp", llvm::None},
+  {R"cpp(
+ // symbol not in index, no header fo

r372708 - [Diagnostics] Do not diagnose unsigned shifts in boolean context (-Wint-in-bool-context)

[David Bolvansky via cfe-commits]

Author: xbolva00
Date: Tue Sep 24 02:14:33 2019
New Revision: 372708

URL: http://llvm.org/viewvc/llvm-project?rev=372708&view=rev
Log:
[Diagnostics] Do not diagnose unsigned shifts in boolean context 
(-Wint-in-bool-context)

I was looking at old GCC's patch. Current "trunk" version avoids warning for 
unsigned case, GCC warns only for signed shifts.


Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/warn-int-in-bool-context.c

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=372708&r1=372707&r2=372708&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Sep 24 02:14:33 
2019
@@ -5722,10 +5722,10 @@ def note_precedence_conditional_first :
 
 def warn_enum_constant_in_bool_context : Warning<
   "converting the enum constant to a boolean">,
-  InGroup;
+  InGroup, DefaultIgnore;
 def warn_left_shift_in_bool_context : Warning<
   "converting the result of '<<' to a boolean; did you mean '(%0) != 0'?">,
-  InGroup;
+  InGroup, DefaultIgnore;
 def warn_logical_instead_of_bitwise : Warning<
   "use of logical '%0' with constant operand">,
   InGroup>;

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=372708&r1=372707&r2=372708&view=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue Sep 24 02:14:33 2019
@@ -11296,44 +11296,39 @@ static bool isSameWidthConstantConversio
   return true;
 }
 
+static const IntegerLiteral *getIntegerLiteral(Expr *E) {
+  const auto *IL = dyn_cast(E);
+  if (!IL) {
+if (auto *UO = dyn_cast(E)) {
+  if (UO->getOpcode() == UO_Minus)
+return dyn_cast(UO->getSubExpr());
+}
+  }
+
+  return IL;
+}
+
 static void DiagnoseIntInBoolContext(Sema &S, Expr *E) {
   E = E->IgnoreParenImpCasts();
   SourceLocation ExprLoc = E->getExprLoc();
 
   if (const auto *BO = dyn_cast(E)) {
 BinaryOperator::Opcode Opc = BO->getOpcode();
-if (Opc == BO_Shl)
+// Do not diagnose unsigned shifts.
+if (Opc == BO_Shl && E->getType()->isSignedIntegerType())
   S.Diag(ExprLoc, diag::warn_left_shift_in_bool_context) << E;
   }
 
   if (const auto *CO = dyn_cast(E)) {
-const auto *LHS = dyn_cast(CO->getTrueExpr());
-if (!LHS) {
-  if (auto *UO = dyn_cast(CO->getTrueExpr())) {
-if (UO->getOpcode() == UO_Minus)
-  LHS = dyn_cast(UO->getSubExpr());
-if (!LHS)
-  return;
-  } else {
-return;
-  }
-}
-
-const auto *RHS = dyn_cast(CO->getFalseExpr());
-if (!RHS) {
-  if (auto *UO = dyn_cast(CO->getFalseExpr())) {
-if (UO->getOpcode() == UO_Minus)
-  RHS = dyn_cast(UO->getSubExpr());
-if (!RHS)
-  return;
-  } else {
-return;
-  }
-}
-
+const auto *LHS = getIntegerLiteral(CO->getTrueExpr());
+if (!LHS)
+  return;
+const auto *RHS = getIntegerLiteral(CO->getFalseExpr());
+if (!RHS)
+  return;
 if ((LHS->getValue() == 0 || LHS->getValue() == 1) &&
 (RHS->getValue() == 0 || RHS->getValue() == 1))
-  // Do not diagnose common idioms
+  // Do not diagnose common idioms.
   return;
 if (LHS->getValue() != 0 && LHS->getValue() != 0)
   S.Diag(ExprLoc, diag::warn_integer_constants_in_conditional_always_true);

Modified: cfe/trunk/test/Sema/warn-int-in-bool-context.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-int-in-bool-context.c?rev=372708&r1=372707&r2=372708&view=diff
==
--- cfe/trunk/test/Sema/warn-int-in-bool-context.c (original)
+++ cfe/trunk/test/Sema/warn-int-in-bool-context.c Tue Sep 24 02:14:33 2019
@@ -7,6 +7,8 @@
 #define TWO 2
 
 #define SHIFT(l, r) l << r
+#define MM a << a
+#define AF 1 << 7
 
 #ifdef __cplusplus
 typedef bool boolean;
@@ -20,15 +22,25 @@ enum num {
   two,
 };
 
-int test(int a, enum num n) {
+int test(int a, unsigned b, enum num n) {
   boolean r;
-  r = (1 << 3); // expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(1 << 3) != 0'?}}
-  r = TWO << 7; // expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(2 << 7) != 0'?}}
+  r = a << a; // expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(a << a) != 0'?}}
+  r = MM; // expected-warning {{converting the result of '<<' to a boolean; 
did you mean '(a << a) != 0'?}}
+  r = (1 << 7); // expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(1 << 7) != 0'?}}
+  r = 2UL << 2;

r372709 - [NFC] Update test after r372708

[David Bolvansky via cfe-commits]

Author: xbolva00
Date: Tue Sep 24 02:24:48 2019
New Revision: 372709

URL: http://llvm.org/viewvc/llvm-project?rev=372709&view=rev
Log:
[NFC] Update test after r372708

Modified:
cfe/trunk/test/SemaCXX/cxx2a-explicit-bool.cpp

Modified: cfe/trunk/test/SemaCXX/cxx2a-explicit-bool.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/cxx2a-explicit-bool.cpp?rev=372709&r1=372708&r2=372709&view=diff
==
--- cfe/trunk/test/SemaCXX/cxx2a-explicit-bool.cpp (original)
+++ cfe/trunk/test/SemaCXX/cxx2a-explicit-bool.cpp Tue Sep 24 02:24:48 2019
@@ -20,7 +20,7 @@ namespace special_cases
 template
 struct A {
 // expected-note@-1+ {{candidate constructor}}
-  explicit(1 << a)  // expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(1 << -1) != 0'?}}
+  explicit(1 << a)
 // expected-note@-1 {{negative shift count -1}}
 // expected-error@-2 {{explicit specifier argument is not a constant 
expression}}
   A(int);


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

[PATCH] D67949: [clang-format] [PR36858] Add missing .hh and .cs extensions from python support utilities

[Manuel Klimek via Phabricator via cfe-commits]

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

lg


Repository:
  rC Clang

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

https://reviews.llvm.org/D67949



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

[PATCH] D53137: Scalable vector core instruction support + size queries

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

> I suspect 'ScalableSize' is the wrong term now; 'TypeSize' may be better. 
> Thoughts?

I agree, TypeSize sounds better. Maybe we can replace the public constructor 
with 2 static methods, TypeSize::Fixed(Size) and TypeSize::Scalable(Size), so 
we don't always have to spell out /* Scalable =*/.




Comment at: llvm/include/llvm/IR/DataLayout.h:454
+auto BaseSize = getTypeSizeInBits(Ty);
+return { (BaseSize.getKnownMinSize() + 7) / 8, BaseSize.isScalable() };
   }

We already overload operator /, why not overload + as well so we don't have to 
change the body of this method?



Comment at: llvm/include/llvm/IR/DataLayout.h:487
+auto BaseSize = getTypeStoreSize(Ty);
+uint64_t MinAlignedSize = alignTo(BaseSize.getKnownMinSize(),
+  getABITypeAlignment(Ty));

Can we add a version of alignTo that works with ScalableSize instead?



Comment at: llvm/include/llvm/IR/DataLayout.h:656
+ 
getTypeSizeInBits(VTy->getElementType()).getKnownMinSize();
+return ScalableSize(MinBits, EltCnt.Scalable);
   }

Maybe just return VTy->getElementCount() * 
getTypeSizeInBits(VTy->getElementType()).getFixedSize().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137



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

[clang-tools-extra] r372711 - [clang-tidy] Add bugprone-infinite-loop.rst from D64736 to fix buildbot

[Fangrui Song via cfe-commits]

Author: maskray
Date: Tue Sep 24 02:32:00 2019
New Revision: 372711

URL: http://llvm.org/viewvc/llvm-project?rev=372711&view=rev
Log:
[clang-tidy] Add bugprone-infinite-loop.rst from D64736 to fix buildbot

Added:
clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-infinite-loop.rst

Added: clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-infinite-loop.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-infinite-loop.rst?rev=372711&view=auto
==
--- clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-infinite-loop.rst 
(added)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-infinite-loop.rst 
Tue Sep 24 02:32:00 2019
@@ -0,0 +1,32 @@
+.. title:: clang-tidy - bugprone-infinite-loop
+
+bugprone-infinite-loop
+==
+
+Finds obvious infinite loops (loops where the condition variable is not changed
+at all).
+
+Finding infinite loops is well-known to be impossible (halting problem).
+However, it is possible to detect some obvious infinite loops, for example, if
+the loop condition is not changed. This check detects such loops. A loop is
+considered infinite if it does not have any loop exit statement (``break``,
+``continue``, ``goto``, ``return``, ``throw`` or a call to a function called as
+``[[noreturn]]``) and all of the following conditions hold for every variable 
in
+the condition:
+
+- It is a local variable.
+- It has no reference or pointer aliases
+- It is not a structure or class member.
+
+Furthermore, the condition must not contain a function call to consider the 
loop
+infinite since functions may return different values for different calls.
+
+For example, the following loop is considered infinite `i` is not changed in
+the body:
+
+.. code-block:: c++
+
+  int i = 0, j = 0;
+  while (i < 10) {
+++j;
+  }


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

[clang-tools-extra] r372714 - [modularize] Fix compilation warning after r372681

[Mikael Holmen via cfe-commits]

Author: uabelho
Date: Tue Sep 24 02:44:55 2019
New Revision: 372714

URL: http://llvm.org/viewvc/llvm-project?rev=372714&view=rev
Log:
[modularize] Fix compilation warning after r372681

In r372681 lang_cxx_11 and lang_cxx_14 were added to LanguageIDs
but they were not handled in the switch in VisitLinkageSpecDecl in
Modularize.cpp so at clang 8 complained with

/data/repo/master/clang-tools-extra/modularize/Modularize.cpp:583:13: error: 
enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch 
[-Werror,-Wswitch]
switch (D->getLanguage()) {
^
1 error generated.

With this patch we now treat lang_cxx, lang_cxx_11 and lang_cxx_14 the
same way in the switch in VisitLinkageSpecDecl.

Modified:
clang-tools-extra/trunk/modularize/Modularize.cpp

Modified: clang-tools-extra/trunk/modularize/Modularize.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/modularize/Modularize.cpp?rev=372714&r1=372713&r2=372714&view=diff
==
--- clang-tools-extra/trunk/modularize/Modularize.cpp (original)
+++ clang-tools-extra/trunk/modularize/Modularize.cpp Tue Sep 24 02:44:55 2019
@@ -585,6 +585,8 @@ public:
   LinkageLabel = "extern \"C\" {}";
   break;
 case LinkageSpecDecl::lang_cxx:
+case LinkageSpecDecl::lang_cxx_11:
+case LinkageSpecDecl::lang_cxx_14:
   LinkageLabel = "extern \"C++\" {}";
   break;
 }


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

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 221492.
hokein marked 2 inline comments as done.
hokein added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67496

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Macro.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -425,8 +425,8 @@
   // Tokens that share a source range but have conflicting Kinds are not
   // highlighted.
   R"cpp(
-  #define DEF_MULTIPLE(X) namespace X { class X { int X; }; }
-  #define DEF_CLASS(T) class T {};
+  #define $Macro[[DEF_MULTIPLE]](X) namespace X { class X { int X; }; }
+  #define $Macro[[DEF_CLASS]](T) class T {};
   // Preamble ends.
   $Macro[[DEF_MULTIPLE]](XYZ);
   $Macro[[DEF_MULTIPLE]](XYZW);
@@ -465,8 +465,8 @@
   }
 )cpp",
   R"cpp(
-  #define fail(expr) expr
-  #define assert(COND) if (!(COND)) { fail("assertion failed" #COND); }
+  #define $Macro[[fail]](expr) expr
+  #define $Macro[[assert]](COND) if (!(COND)) { fail("assertion failed" #COND); }
   // Preamble ends.
   $Primitive[[int]] $Variable[[x]];
   $Primitive[[int]] $Variable[[y]];
Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -229,8 +229,8 @@
 
 TEST(ParsedASTTest, CollectsMainFileMacroExpansions) {
   Annotations TestCase(R"cpp(
-#define MACRO_ARGS(X, Y) X Y
-// - premable ends, macros inside preamble are not considered in main file.
+#define ^MACRO_ARGS(X, Y) X Y
+// - preamble ends
 ^ID(int A);
 // Macro arguments included.
 ^MACRO_ARGS(^MACRO_ARGS(^MACRO_EXP(int), A), ^ID(= 2));
@@ -270,12 +270,11 @@
 int D = DEF;
   )cpp";
   ParsedAST AST = TU.build();
-  const std::vector &MacroExpansionLocations = AST.getMacros();
   std::vector MacroExpansionPositions;
-  for (const auto &L : MacroExpansionLocations)
-MacroExpansionPositions.push_back(
-sourceLocToPosition(AST.getSourceManager(), L));
-  EXPECT_EQ(MacroExpansionPositions, TestCase.points());
+  for (const auto &R : AST.getMacros().Ranges)
+MacroExpansionPositions.push_back(R.start);
+  EXPECT_THAT(MacroExpansionPositions,
+  testing::UnorderedElementsAreArray(TestCase.points()));
 }
 
 } // namespace
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -110,8 +110,9 @@
 TraverseAST(AST.getASTContext());
 // Add highlightings for macro expansions as they are not traversed by the
 // visitor.
-for (SourceLocation Loc : AST.getMacros())
-  addToken(Loc, HighlightingKind::Macro);
+for (const auto &M : AST.getMacros().Ranges)
+  Tokens.push_back({HighlightingKind::Macro, M});
+// addToken(Loc, HighlightingKind::Macro);
 // Initializer lists can give duplicates of tokens, therefore all tokens
 // must be deduplicated.
 llvm::sort(Tokens);
Index: clang-tools-extra/clangd/Preamble.h
===
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -26,6 +26,7 @@
 #include "Diagnostics.h"
 #include "FS.h"
 #include "Headers.h"
+#include "Macro.h"
 #include "index/CanonicalIncludes.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Tooling/CompilationDatabase.h"
@@ -42,12 +43,6 @@
 /// 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(PrecompiledPreamble Preamble, std::vector Diags,
-   IncludeStructure Includes,
-   std::vector MainFileMacros,
-   std::unique_ptr StatCache,
-   CanonicalIncludes CanonIncludes);
-
   tooling::CompileCommand CompileCommand;
   PrecompiledPreamble Preamble;
   std::vector Diags;
@@ -57,11 +52,16 @@
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These 

[PATCH] D67613: [DWARF-5] Support for DWARF-5 C++ language tags

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

In D67613#1680176 , @uabelho wrote:

> In D67613#1679879 , @xbolva00 wrote:
>
> > /srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13:
> >  warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in 
> > switch [-Wswitch]
> >  1 warning generated.
>
>
> We also get that warning when compiling with clang 8:
>
>   ../../clang-tools-extra/modularize/Modularize.cpp:583:13: error: 
> enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch 
> [-Werror,-Wswitch]
>   switch (D->getLanguage()) {
>   ^
>   1 error generated.
>


I did something about it in r372714. Please take a look and see if it should be 
fixed differently.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67613



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

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/Macro.h:67
+  bool InMainFile = true;
+  std::vector &MainFileMacros;
+};

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Could we model in a way that avoids duplicating macro names on each 
> > > occurrence?
> > > We had `StringSet Names` and `vector Locations`, let's 
> > > keep it in the same way.
> > > 
> > > We could group this into a struct to reduce boilerplate of transferring 
> > > it around, obviously
> > > ```
> > > struct MainFileMacros {
> > >   StringSet Names;
> > >   vector Locations;
> > > };
> > > ```
> > yes, we don't need the name and location reletionship in this patch, but 
> > we'd need this when implementing xrefs for macros. do you think we should 
> > keep the same way, and do the change when we start implementing xrefs for 
> > macros?
> Cross-references have to be more fine-grained and having only a name is not 
> enough. Let's address this separately when/if we actually need this in xrefs.
> In particular, we want to handle macro re-definitions:
> ```
> #define FOO 123 // #1
> int a = FOO; // a reference to #1
> 
> #define FOO 234 // #2
> int b = FOO; // a reference to #2
> ```
> Current model will merge these two together as both names are the same.
good point, agreed. I think we probably use the symbol id for macros, but it is 
out-scope of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67496



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

[clang-tools-extra] r372715 - [clang-tidy][test] Add -fexceptions to bugprone-infinite-loop.test

[Fangrui Song via cfe-commits]

Author: maskray
Date: Tue Sep 24 02:55:35 2019
New Revision: 372715

URL: http://llvm.org/viewvc/llvm-project?rev=372715&view=rev
Log:
[clang-tidy][test] Add -fexceptions to bugprone-infinite-loop.test

This fixes llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast.

-fexceptions is disabled by default on XCore and PS4.

Modified:
clang-tools-extra/trunk/test/clang-tidy/bugprone-infinite-loop.cpp

Modified: clang-tools-extra/trunk/test/clang-tidy/bugprone-infinite-loop.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/bugprone-infinite-loop.cpp?rev=372715&r1=372714&r2=372715&view=diff
==
--- clang-tools-extra/trunk/test/clang-tidy/bugprone-infinite-loop.cpp 
(original)
+++ clang-tools-extra/trunk/test/clang-tidy/bugprone-infinite-loop.cpp Tue Sep 
24 02:55:35 2019
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-infinite-loop %t
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fexceptions
 
 void simple_infinite_loop1() {
   int i = 0;


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

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.

A ton of small NITs too, but they're all small details.




Comment at: clang-tools-extra/clangd/Macro.h:1
+//===--- Macro.h -*- 
C++-*-===//
+//

NIT: Maybe rename to `CollectMacros.h`?

`Macro.h` looks a bit too generic and may start being a place for more helpers. 
However, the current file is pretty well-structured.




Comment at: clang-tools-extra/clangd/Macro.h:23
+  llvm::StringSet<> Names;
+  std::vector Ranges;
+};

Could you add a comment that we have to convert to `Range` early because source 
manager from preamble is not available when we build the AST?



Comment at: clang-tools-extra/clangd/Macro.h:26
+
+// Collects macro definitions and expansions in the main file. it is used to:
+//   - collect macros in the preamble section of the main file (in 
Preamble.cpp)

NIT: a typo: in the main file. **It** is used ...



Comment at: clang-tools-extra/clangd/Macro.h:28
+//   - collect macros in the preamble section of the main file (in 
Preamble.cpp)
+//   - collect macros after the preamble of the main file (in ParsedAST.cpp)
+class CollectMainFileMacros : public PPCallbacks {

NIT: use `///` to enable doxygen comments



Comment at: clang-tools-extra/clangd/Macro.h:65
+  const LangOptions &LangOpts;
+  bool InMainFile = true;
+  MainFileMacros &Out;

NIT: It's probably safer to set 'no' by default.
Normally the first thing visited by the parser is `` buffer, not the 
main file.

OTOH, `FileChanged` should be called first in any case, so it does not matter.



Comment at: clang-tools-extra/clangd/ParsedAST.h:104
 std::unique_ptr Action, syntax::TokenBuffer Tokens,
-std::vector MainFileMacroExpLocs,
+MainFileMacros MainFileMacroExpLocs,
 std::vector LocalTopLevelDecls, std::vector Diags,

NIT: rename parameter to `Macros`



Comment at: clang-tools-extra/clangd/ParsedAST.h:124
 
-  /// The start locations of all macro definitions/expansions spelled **after**
-  /// preamble.
-  /// Does not include locations from inside other macro expansions.
-  std::vector MacroIdentifierLocs;
+  /// All macro definitions/expansions in the main file.
+  MainFileMacros Macros;

NIT: change `definitions/expansions` to `definitions and expansion`



Comment at: clang-tools-extra/clangd/Preamble.cpp:34
 
-  std::vector takeMainFileMacros() {
-return std::move(MainFileMacros);
-  }
+  MainFileMacros takeMainFileMacros() { return std::move(Macros); }
 

NIT: rename to `takeMacros`



Comment at: clang-tools-extra/clangd/Preamble.cpp:73
+  const clang::LangOptions *LangOpts = nullptr;
   SourceManager *SourceMgr = nullptr;
 };

NIT: add **const** to `SourceManager` too, for consistency
(if possible)



Comment at: clang-tools-extra/clangd/Preamble.h:61
+
+  PreambleData(PrecompiledPreamble Preamble, std::vector Diags,
+   IncludeStructure Includes, MainFileMacros Macros,

There seems to be no reason for moving constructor to the bottom of the class. 
Maybe keep as is?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67496



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

[PATCH] D67613: [DWARF-5] Support for DWARF-5 C++ language tags

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX added a comment.

In D67613#1680176 , @uabelho wrote:

> In D67613#1679879 , @xbolva00 wrote:
>
> > /srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13:
> >  warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in 
> > switch [-Wswitch]
> >  1 warning generated.
>
>
> We also get that warning when compiling with clang 8:
>
>   ../../clang-tools-extra/modularize/Modularize.cpp:583:13: error: 
> enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch 
> [-Werror,-Wswitch]
>   switch (D->getLanguage()) {
>   ^
>   1 error generated.
>




In D67613#1680398 , @uabelho wrote:

> In D67613#1680176 , @uabelho wrote:
>
> > In D67613#1679879 , @xbolva00 
> > wrote:
> >
> > > /srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13:
> > >  warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled 
> > > in switch [-Wswitch]
> > >  1 warning generated.
> >
> >
> > We also get that warning when compiling with clang 8:
> >
> >   ../../clang-tools-extra/modularize/Modularize.cpp:583:13: error: 
> > enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch 
> > [-Werror,-Wswitch]
> >   switch (D->getLanguage()) {
> >   ^
> >   1 error generated.
> >
>
>
> I did something about it in r372714. Please take a look and see if it should 
> be fixed differently.


Hi Thanks for doing this, I got occupied by something else.  that seems good.
Thanks again!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67613



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

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 221499.
usaxena95 marked 7 inline comments as done.
usaxena95 added a comment.

Addressed comments:
Added client/server capabilities. 
Made lit test smaller.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/test/selection-range.test

Index: clang-tools-extra/clangd/test/selection-range.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/selection-range.test
@@ -0,0 +1,39 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void func() {\n}"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/selectionRange","params":{"textDocument":{"uri":"test:///main.cpp"},"positions":[{"line":1,"character":0}]}}
+#  CHECK:  "id": 1
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "parent": {
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 1,
+# CHECK-NEXT:"line": 1
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 0,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 1,
+# CHECK-NEXT:  "line": 1
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 12,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ]
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,7 @@
 # CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
+# CHECK-NEXT:  "selectionRangeProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
 # CHECK-NEXT:  "(",
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -30,6 +30,7 @@
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -416,6 +417,10 @@
   /// textDocument.semanticHighlightingCapabilities.semanticHighlighting
   bool SemanticHighlighting = false;
 
+  /// Client supports semantic selection.
+  /// textDocument.selectionRange
+  bool SemanticSelection = false;
+
   /// Supported encodings for LSP character offsets. (clangd extension).
   llvm::Optional> offsetEncoding;
 
@@ -1222,6 +1227,31 @@
 };
 llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting);
 
+struct SelectionRangeParams {
+  /// The text document.
+  TextDocumentIdentifier textDocument;
+
+  /// The positions inside the text document.
+  std::vector positions;
+};
+bool fromJSON(const llvm::json::Value &, SelectionRangeParams &);
+
+struct SelectionRange {
+  /**
+   * The [range](#Range) of this selection range.
+   */
+  Range range;
+  /**
+   * The parent selection range containing this range. Therefore `parent.range`
+   * must contain `this.range`.
+   */
+  llvm::Optional> parent;
+
+  SelectionRange() = default;
+  SelectionRange(SelectionRange &&) = default;
+};
+llvm::json::Value toJSON(const SelectionRange &);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -15,6 +15,7 @@
 #include "URI.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -335,6 +336,9 @@
   if (auto RenameSupport = Rename->getBoolean("prepareSupport"))
 R.RenamePrepareSupport = *RenameSupport;
 }
+if (auto *SemanticSelection = TextDocument->get

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 added a comment.

I am not sure adding client capability is useful here. I have not used the 
client capability for selectionRange anywhere and I think we can remove it. 
WDYT ?




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1131
+Callback> Reply) {
+  if (Params.positions.size() != 1) {
+elog("{0} positions provided to SelectionRange. Supports exactly one "

ilya-biryukov wrote:
> hokein wrote:
> > maybe add an `assert(!Params.positions.empty())`. I think we should not run 
> > into this case.
> But `Params` comes to clangd over LSP, right?
> That means `assert` can fire in case of bad inputs over LSP to clangd.
> Bad inputs over LSP should never crash clangd.
Yes this comes from the client and can be a bad input. We should just return 
error and not crash in such case.



Comment at: clang-tools-extra/clangd/Protocol.h:1241
+llvm::json::Value toJSON(const SelectionRange &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SelectionRange &);
+

hokein wrote:
> does this operator get used in this patch?
No. Removed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720



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

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 221501.
hokein marked 10 inline comments as done.
hokein added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67496

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CollectMacros.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -425,8 +425,8 @@
   // Tokens that share a source range but have conflicting Kinds are not
   // highlighted.
   R"cpp(
-  #define DEF_MULTIPLE(X) namespace X { class X { int X; }; }
-  #define DEF_CLASS(T) class T {};
+  #define $Macro[[DEF_MULTIPLE]](X) namespace X { class X { int X; }; }
+  #define $Macro[[DEF_CLASS]](T) class T {};
   // Preamble ends.
   $Macro[[DEF_MULTIPLE]](XYZ);
   $Macro[[DEF_MULTIPLE]](XYZW);
@@ -465,8 +465,8 @@
   }
 )cpp",
   R"cpp(
-  #define fail(expr) expr
-  #define assert(COND) if (!(COND)) { fail("assertion failed" #COND); }
+  #define $Macro[[fail]](expr) expr
+  #define $Macro[[assert]](COND) if (!(COND)) { fail("assertion failed" #COND); }
   // Preamble ends.
   $Primitive[[int]] $Variable[[x]];
   $Primitive[[int]] $Variable[[y]];
Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -229,8 +229,8 @@
 
 TEST(ParsedASTTest, CollectsMainFileMacroExpansions) {
   Annotations TestCase(R"cpp(
-#define MACRO_ARGS(X, Y) X Y
-// - premable ends, macros inside preamble are not considered in main file.
+#define ^MACRO_ARGS(X, Y) X Y
+// - preamble ends
 ^ID(int A);
 // Macro arguments included.
 ^MACRO_ARGS(^MACRO_ARGS(^MACRO_EXP(int), A), ^ID(= 2));
@@ -270,12 +270,11 @@
 int D = DEF;
   )cpp";
   ParsedAST AST = TU.build();
-  const std::vector &MacroExpansionLocations = AST.getMacros();
   std::vector MacroExpansionPositions;
-  for (const auto &L : MacroExpansionLocations)
-MacroExpansionPositions.push_back(
-sourceLocToPosition(AST.getSourceManager(), L));
-  EXPECT_EQ(MacroExpansionPositions, TestCase.points());
+  for (const auto &R : AST.getMacros().Ranges)
+MacroExpansionPositions.push_back(R.start);
+  EXPECT_THAT(MacroExpansionPositions,
+  testing::UnorderedElementsAreArray(TestCase.points()));
 }
 
 } // namespace
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -110,8 +110,9 @@
 TraverseAST(AST.getASTContext());
 // Add highlightings for macro expansions as they are not traversed by the
 // visitor.
-for (SourceLocation Loc : AST.getMacros())
-  addToken(Loc, HighlightingKind::Macro);
+for (const auto &M : AST.getMacros().Ranges)
+  Tokens.push_back({HighlightingKind::Macro, M});
+// addToken(Loc, HighlightingKind::Macro);
 // Initializer lists can give duplicates of tokens, therefore all tokens
 // must be deduplicated.
 llvm::sort(Tokens);
Index: clang-tools-extra/clangd/Preamble.h
===
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -22,6 +22,7 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_PREAMBLE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_PREAMBLE_H
 
+#include "CollectMacros.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "FS.h"
@@ -43,8 +44,7 @@
 /// be obtained during parsing must be eagerly captured and stored here.
 struct PreambleData {
   PreambleData(PrecompiledPreamble Preamble, std::vector Diags,
-   IncludeStructure Includes,
-   std::vector MainFileMacros,
+   IncludeStructure Includes, MainFileMacros Macros,
std::unique_ptr StatCache,
CanonicalIncludes CanonIncludes);
 
@@ -57,7 +57,7 @@
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These should be treated as main-file entities e.g. for code completion.
-  std::vector MainFileMacros;
+  MainFileMacros Macro

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D67720#1680460 , @usaxena95 wrote:

> I am not sure adding client capability is useful here. I have not used the 
> client capability for selectionRange anywhere and I think we can remove it. 
>  WDYT ?


The `SelectionRangeClientCapabilities` determines what should the LSP server 
send the client, if it is true, clangd should send 
`SelectionRangeRegistrationOptions`. But looking at the current specification, 
it doesn't seem to add too much value. I think we can just simplify return a 
bool for now (as you did in this patch).




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1131
+Callback> Reply) {
+  if (Params.positions.size() != 1) {
+elog("{0} positions provided to SelectionRange. Supports exactly one "

usaxena95 wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > maybe add an `assert(!Params.positions.empty())`. I think we should not 
> > > run into this case.
> > But `Params` comes to clangd over LSP, right?
> > That means `assert` can fire in case of bad inputs over LSP to clangd.
> > Bad inputs over LSP should never crash clangd.
> Yes this comes from the client and can be a bad input. We should just return 
> error and not crash in such case.
but the code still doesn't handle the `empty` case?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:141
+  auto *Next = &Result.parent;
+  for (size_t I = 1; I < Ranges.size(); ++I) {
+*Next = std::make_unique();

nit: use for range.



Comment at: clang-tools-extra/clangd/Protocol.h:1248
+   */
+  llvm::Optional> parent;
+

I think we can simplify the code further, using 
`llvm::Optional` should be enough, the parent is null for the 
outer-most range.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720



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

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

[Haojian Wu via Phabricator via cfe-commits]

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

update header guard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67496

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CollectMacros.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -425,8 +425,8 @@
   // Tokens that share a source range but have conflicting Kinds are not
   // highlighted.
   R"cpp(
-  #define DEF_MULTIPLE(X) namespace X { class X { int X; }; }
-  #define DEF_CLASS(T) class T {};
+  #define $Macro[[DEF_MULTIPLE]](X) namespace X { class X { int X; }; }
+  #define $Macro[[DEF_CLASS]](T) class T {};
   // Preamble ends.
   $Macro[[DEF_MULTIPLE]](XYZ);
   $Macro[[DEF_MULTIPLE]](XYZW);
@@ -465,8 +465,8 @@
   }
 )cpp",
   R"cpp(
-  #define fail(expr) expr
-  #define assert(COND) if (!(COND)) { fail("assertion failed" #COND); }
+  #define $Macro[[fail]](expr) expr
+  #define $Macro[[assert]](COND) if (!(COND)) { fail("assertion failed" #COND); }
   // Preamble ends.
   $Primitive[[int]] $Variable[[x]];
   $Primitive[[int]] $Variable[[y]];
Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -229,8 +229,8 @@
 
 TEST(ParsedASTTest, CollectsMainFileMacroExpansions) {
   Annotations TestCase(R"cpp(
-#define MACRO_ARGS(X, Y) X Y
-// - premable ends, macros inside preamble are not considered in main file.
+#define ^MACRO_ARGS(X, Y) X Y
+// - preamble ends
 ^ID(int A);
 // Macro arguments included.
 ^MACRO_ARGS(^MACRO_ARGS(^MACRO_EXP(int), A), ^ID(= 2));
@@ -270,12 +270,11 @@
 int D = DEF;
   )cpp";
   ParsedAST AST = TU.build();
-  const std::vector &MacroExpansionLocations = AST.getMacros();
   std::vector MacroExpansionPositions;
-  for (const auto &L : MacroExpansionLocations)
-MacroExpansionPositions.push_back(
-sourceLocToPosition(AST.getSourceManager(), L));
-  EXPECT_EQ(MacroExpansionPositions, TestCase.points());
+  for (const auto &R : AST.getMacros().Ranges)
+MacroExpansionPositions.push_back(R.start);
+  EXPECT_THAT(MacroExpansionPositions,
+  testing::UnorderedElementsAreArray(TestCase.points()));
 }
 
 } // namespace
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -110,8 +110,9 @@
 TraverseAST(AST.getASTContext());
 // Add highlightings for macro expansions as they are not traversed by the
 // visitor.
-for (SourceLocation Loc : AST.getMacros())
-  addToken(Loc, HighlightingKind::Macro);
+for (const auto &M : AST.getMacros().Ranges)
+  Tokens.push_back({HighlightingKind::Macro, M});
+// addToken(Loc, HighlightingKind::Macro);
 // Initializer lists can give duplicates of tokens, therefore all tokens
 // must be deduplicated.
 llvm::sort(Tokens);
Index: clang-tools-extra/clangd/Preamble.h
===
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -22,6 +22,7 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_PREAMBLE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_PREAMBLE_H
 
+#include "CollectMacros.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "FS.h"
@@ -43,8 +44,7 @@
 /// be obtained during parsing must be eagerly captured and stored here.
 struct PreambleData {
   PreambleData(PrecompiledPreamble Preamble, std::vector Diags,
-   IncludeStructure Includes,
-   std::vector MainFileMacros,
+   IncludeStructure Includes, MainFileMacros Macros,
std::unique_ptr StatCache,
CanonicalIncludes CanonIncludes);
 
@@ -57,7 +57,7 @@
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These should be treated as main-file entities e.g. for code completion.
-  std::vector MainFileMacros;
+  MainFileMacros Macros;
   // Cache of FS operations performed when

[clang-tools-extra] r372725 - [clangd] Collect macros in the preamble region of the main file

[Haojian Wu via cfe-commits]

Author: hokein
Date: Tue Sep 24 04:14:06 2019
New Revision: 372725

URL: http://llvm.org/viewvc/llvm-project?rev=372725&view=rev
Log:
[clangd] Collect macros in the preamble region of the main file

Summary:
- store all macro references in the ParsedAST;
- unify the two variants of CollectMainFileMacros;

Reviewers: ilya-biryukov

Subscribers: MaskRay, jkorous, mgrang, arphaman, kadircet, cfe-commits

Tags: #clang

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

Added:
clang-tools-extra/trunk/clangd/CollectMacros.h
Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/ParsedAST.cpp
clang-tools-extra/trunk/clangd/ParsedAST.h
clang-tools-extra/trunk/clangd/Preamble.cpp
clang-tools-extra/trunk/clangd/Preamble.h
clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
clang-tools-extra/trunk/clangd/unittests/ParsedASTTests.cpp
clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp

Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=372725&r1=372724&r2=372725&view=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Tue Sep 24 04:14:06 2019
@@ -1030,8 +1030,8 @@ void loadMainFilePreambleMacros(const Pr
   PP.getIdentifierTable().getExternalIdentifierLookup();
   if (!PreambleIdentifiers || !PreambleMacros)
 return;
-  for (const auto &MacroName : Preamble.MainFileMacros)
-if (auto *II = PreambleIdentifiers->get(MacroName))
+  for (const auto &MacroName : Preamble.Macros.Names)
+if (auto *II = PreambleIdentifiers->get(MacroName.getKey()))
   if (II->isOutOfDate())
 PreambleMacros->updateOutOfDateIdentifier(*II);
 }

Added: clang-tools-extra/trunk/clangd/CollectMacros.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CollectMacros.h?rev=372725&view=auto
==
--- clang-tools-extra/trunk/clangd/CollectMacros.h (added)
+++ clang-tools-extra/trunk/clangd/CollectMacros.h Tue Sep 24 04:14:06 2019
@@ -0,0 +1,74 @@
+//===--- CollectMacros.h -*- 
C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_COLLECTEDMACROS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COLLECTEDMACROS_H
+
+#include "Protocol.h"
+#include "SourceCode.h"
+#include "clang/Basic/IdentifierTable.h"
+#include "clang/Lex/PPCallbacks.h"
+#include 
+
+namespace clang {
+namespace clangd {
+
+struct MainFileMacros {
+  llvm::StringSet<> Names;
+  // Instead of storing SourceLocation, we have to store the token range 
because
+  // SourceManager from preamble is not available when we build the AST.
+  std::vector Ranges;
+};
+
+/// Collects macro definitions and expansions in the main file. It is used to:
+///  - collect macros in the preamble section of the main file (in 
Preamble.cpp)
+///  - collect macros after the preamble of the main file (in ParsedAST.cpp)
+class CollectMainFileMacros : public PPCallbacks {
+public:
+  explicit CollectMainFileMacros(const SourceManager &SM,
+ const LangOptions &LangOpts,
+ MainFileMacros &Out)
+  : SM(SM), LangOpts(LangOpts), Out(Out) {}
+
+  void FileChanged(SourceLocation Loc, FileChangeReason,
+   SrcMgr::CharacteristicKind, FileID) override {
+InMainFile = isInsideMainFile(Loc, SM);
+  }
+
+  void MacroDefined(const Token &MacroName, const MacroDirective *MD) override 
{
+add(MacroName, MD->getMacroInfo());
+  }
+
+  void MacroExpands(const Token &MacroName, const MacroDefinition &MD,
+SourceRange Range, const MacroArgs *Args) override {
+add(MacroName, MD.getMacroInfo());
+  }
+
+private:
+  void add(const Token &MacroNameTok, const MacroInfo *MI) {
+if (!InMainFile)
+  return;
+auto Loc = MacroNameTok.getLocation();
+if (Loc.isMacroID())
+  return;
+
+if (auto Range = getTokenRange(SM, LangOpts, MacroNameTok.getLocation())) {
+  Out.Names.insert(MacroNameTok.getIdentifierInfo()->getName());
+  Out.Ranges.push_back(*Range);
+}
+  }
+  const SourceManager &SM;
+  const LangOptions &LangOpts;
+  bool InMainFile = true;
+  MainFileMacros &Out;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_COLLECTEDMACROS_H

Modified: clang-tools-extra/trunk/clangd/ParsedAST.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Parse

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372725: [clangd] Collect macros in the preamble region of 
the main file (authored by hokein, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67496?vs=221504&id=221505#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67496

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/CollectMacros.h
  clang-tools-extra/trunk/clangd/ParsedAST.cpp
  clang-tools-extra/trunk/clangd/ParsedAST.h
  clang-tools-extra/trunk/clangd/Preamble.cpp
  clang-tools-extra/trunk/clangd/Preamble.h
  clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp
  clang-tools-extra/trunk/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -1030,8 +1030,8 @@
   PP.getIdentifierTable().getExternalIdentifierLookup();
   if (!PreambleIdentifiers || !PreambleMacros)
 return;
-  for (const auto &MacroName : Preamble.MainFileMacros)
-if (auto *II = PreambleIdentifiers->get(MacroName))
+  for (const auto &MacroName : Preamble.Macros.Names)
+if (auto *II = PreambleIdentifiers->get(MacroName.getKey()))
   if (II->isOutOfDate())
 PreambleMacros->updateOutOfDateIdentifier(*II);
 }
Index: clang-tools-extra/trunk/clangd/Preamble.h
===
--- clang-tools-extra/trunk/clangd/Preamble.h
+++ clang-tools-extra/trunk/clangd/Preamble.h
@@ -22,6 +22,7 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_PREAMBLE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_PREAMBLE_H
 
+#include "CollectMacros.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "FS.h"
@@ -43,8 +44,7 @@
 /// be obtained during parsing must be eagerly captured and stored here.
 struct PreambleData {
   PreambleData(PrecompiledPreamble Preamble, std::vector Diags,
-   IncludeStructure Includes,
-   std::vector MainFileMacros,
+   IncludeStructure Includes, MainFileMacros Macros,
std::unique_ptr StatCache,
CanonicalIncludes CanonIncludes);
 
@@ -57,7 +57,7 @@
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These should be treated as main-file entities e.g. for code completion.
-  std::vector MainFileMacros;
+  MainFileMacros Macros;
   // Cache of FS operations performed when building the preamble.
   // When reusing a preamble, this cache can be consumed to save IO.
   std::unique_ptr StatCache;
Index: clang-tools-extra/trunk/clangd/ParsedAST.cpp
===
--- clang-tools-extra/trunk/clangd/ParsedAST.cpp
+++ clang-tools-extra/trunk/clangd/ParsedAST.cpp
@@ -98,33 +98,6 @@
   std::vector TopLevelDecls;
 };
 
-// This collects macro expansions/definitions in the main file.
-// (Contrast with CollectMainFileMacros in Preamble.cpp, which collects macro
-// *definitions* in the preamble region of the main file).
-class CollectMainFileMacros : public PPCallbacks {
-  const SourceManager &SM;
-  std::vector &MainFileMacroLocs;
-
-  void addLoc(SourceLocation Loc) {
-if (!Loc.isMacroID() && isInsideMainFile(Loc, SM))
-  MainFileMacroLocs.push_back(Loc);
-  }
-
-public:
-  CollectMainFileMacros(const SourceManager &SM,
-std::vector &MainFileMacroLocs)
-  : SM(SM), MainFileMacroLocs(MainFileMacroLocs) {}
-
-  void MacroDefined(const Token &MacroNameTok,
-const MacroDirective *MD) override {
-addLoc(MacroNameTok.getLocation());
-  }
-  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
-SourceRange Range, const MacroArgs *Args) override {
-addLoc(MacroNameTok.getLocation());
-  }
-};
-
 // When using a preamble, only preprocessor events outside its bounds are seen.
 // This is almost what we want: replaying transitive preprocessing wastes time.
 // However this confuses clang-tidy checks: they don't see any #includes!
@@ -362,11 +335,14 @@
   // (We can't *just* use the replayed includes, they don't have Resolved path).
   Clang->getPreprocessor().addPPCallbacks(
   collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
-  // Collect the macro expansions in the main file.
-  std::vector MainFileMacroExpLocs;
+  // Copy over the macros in the preamble region of the main file, and combine
+  // with non-preamble macros below.
+  MainFileMacros Macros;
+  if (Preamble)
+Macros = Preamble->Macros;
   Clang->getPreproc

[PATCH] D66937: [clangd] Fix the stale documentation about background indexing.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 221508.
hokein marked 8 inline comments as done.
hokein added a comment.
Herald added a subscriber: usaxena95.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66937

Files:
  clang-tools-extra/docs/clangd/Installation.rst


Index: clang-tools-extra/docs/clangd/Installation.rst
===
--- clang-tools-extra/docs/clangd/Installation.rst
+++ clang-tools-extra/docs/clangd/Installation.rst
@@ -352,20 +352,28 @@
 Creating this file by hand is a reasonable place to start if your project is
 quite simple.
 
-Project-wide Index
-==
+Background Indexing
+===
 
-By default clangd only has a view on symbols coming from files you are
-currently editing. You can extend this view to whole project by providing a
-project-wide index to clangd.  There are two ways to do this.
+clangd builds an incremental index of your project (all files listed in the
+compilation database). The index improves code navigation features 
(go-to-definition,
+find-references) and code completion.
 
-- Pass an experimental `-background-index` command line argument.  With
-  this feature enabled, clangd incrementally builds an index of projects
-  that you work on and uses the just-built index automatically.
+- the index is saved to the ``.clangd/index`` in the project root;
+- clangd only uses idle cores to build the index, you can limit the total
+  amount of cores by passing the `-j=` flag;
+- background indexing can be disabled by the ``--background-index=false`` flag;
+  if it is disabled, clangd doesn't have a global view of the whole project, it
+  only has a view on symbols coming from files you are currently editing;
 
-- Generate an index file using `clangd-indexer
+Build Index Manually
+
+
+**DISCLAMER: This is mainly for clangd developers.**
+
+There is a `clangd-indexer
   
`__
-  Then you can pass generated index file to clangd using
-  `-index-file=/path/to/index_file`.  *Note that clangd-indexer isn't
+which generates an index file for your project. To use the index, pass the flag
+`-index=file=/path/to/index_file` to clangd. *Note that clangd-indexer isn't
   included alongside clangd in the Debian clang-tools package. You will
   likely have to build it from source to use this option.*


Index: clang-tools-extra/docs/clangd/Installation.rst
===
--- clang-tools-extra/docs/clangd/Installation.rst
+++ clang-tools-extra/docs/clangd/Installation.rst
@@ -352,20 +352,28 @@
 Creating this file by hand is a reasonable place to start if your project is
 quite simple.
 
-Project-wide Index
-==
+Background Indexing
+===
 
-By default clangd only has a view on symbols coming from files you are
-currently editing. You can extend this view to whole project by providing a
-project-wide index to clangd.  There are two ways to do this.
+clangd builds an incremental index of your project (all files listed in the
+compilation database). The index improves code navigation features (go-to-definition,
+find-references) and code completion.
 
-- Pass an experimental `-background-index` command line argument.  With
-  this feature enabled, clangd incrementally builds an index of projects
-  that you work on and uses the just-built index automatically.
+- the index is saved to the ``.clangd/index`` in the project root;
+- clangd only uses idle cores to build the index, you can limit the total
+  amount of cores by passing the `-j=` flag;
+- background indexing can be disabled by the ``--background-index=false`` flag;
+  if it is disabled, clangd doesn't have a global view of the whole project, it
+  only has a view on symbols coming from files you are currently editing;
 
-- Generate an index file using `clangd-indexer
+Build Index Manually
+
+
+**DISCLAMER: This is mainly for clangd developers.**
+
+There is a `clangd-indexer
   `__
-  Then you can pass generated index file to clangd using
-  `-index-file=/path/to/index_file`.  *Note that clangd-indexer isn't
+which generates an index file for your project. To use the index, pass the flag
+`-index=file=/path/to/index_file` to clangd. *Note that clangd-indexer isn't
   included alongside clangd in the Debian clang-tools package. You will
   likely have to build it from source to use this option.*
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66937: [clangd] Fix the stale documentation about background indexing.

[Haojian Wu via Phabricator via cfe-commits]

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

fix a typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66937

Files:
  clang-tools-extra/docs/clangd/Installation.rst


Index: clang-tools-extra/docs/clangd/Installation.rst
===
--- clang-tools-extra/docs/clangd/Installation.rst
+++ clang-tools-extra/docs/clangd/Installation.rst
@@ -352,20 +352,28 @@
 Creating this file by hand is a reasonable place to start if your project is
 quite simple.
 
-Project-wide Index
-==
+Background Indexing
+===
 
-By default clangd only has a view on symbols coming from files you are
-currently editing. You can extend this view to whole project by providing a
-project-wide index to clangd.  There are two ways to do this.
+clangd builds an incremental index of your project (all files listed in the
+compilation database). The index improves code navigation features 
(go-to-definition,
+find-references) and code completion.
 
-- Pass an experimental `-background-index` command line argument.  With
-  this feature enabled, clangd incrementally builds an index of projects
-  that you work on and uses the just-built index automatically.
+- the index is saved to the ``.clangd/index`` in the project root;
+- clangd only uses idle cores to build the index, you can limit the total
+  amount of cores by passing the `-j=` flag;
+- background indexing can be disabled by the ``--background-index=false`` flag;
+  if it is disabled, clangd doesn't have a global view of the whole project, it
+  only has a view on symbols coming from files you are currently editing;
 
-- Generate an index file using `clangd-indexer
+Build Index Manually
+
+
+**DISCLAIMER: This is mainly for clangd developers.**
+
+There is a `clangd-indexer
   
`__
-  Then you can pass generated index file to clangd using
-  `-index-file=/path/to/index_file`.  *Note that clangd-indexer isn't
+which generates an index file for your project. To use the index, pass the flag
+`-index=file=/path/to/index_file` to clangd. *Note that clangd-indexer isn't
   included alongside clangd in the Debian clang-tools package. You will
   likely have to build it from source to use this option.*


Index: clang-tools-extra/docs/clangd/Installation.rst
===
--- clang-tools-extra/docs/clangd/Installation.rst
+++ clang-tools-extra/docs/clangd/Installation.rst
@@ -352,20 +352,28 @@
 Creating this file by hand is a reasonable place to start if your project is
 quite simple.
 
-Project-wide Index
-==
+Background Indexing
+===
 
-By default clangd only has a view on symbols coming from files you are
-currently editing. You can extend this view to whole project by providing a
-project-wide index to clangd.  There are two ways to do this.
+clangd builds an incremental index of your project (all files listed in the
+compilation database). The index improves code navigation features (go-to-definition,
+find-references) and code completion.
 
-- Pass an experimental `-background-index` command line argument.  With
-  this feature enabled, clangd incrementally builds an index of projects
-  that you work on and uses the just-built index automatically.
+- the index is saved to the ``.clangd/index`` in the project root;
+- clangd only uses idle cores to build the index, you can limit the total
+  amount of cores by passing the `-j=` flag;
+- background indexing can be disabled by the ``--background-index=false`` flag;
+  if it is disabled, clangd doesn't have a global view of the whole project, it
+  only has a view on symbols coming from files you are currently editing;
 
-- Generate an index file using `clangd-indexer
+Build Index Manually
+
+
+**DISCLAIMER: This is mainly for clangd developers.**
+
+There is a `clangd-indexer
   `__
-  Then you can pass generated index file to clangd using
-  `-index-file=/path/to/index_file`.  *Note that clangd-indexer isn't
+which generates an index file for your project. To use the index, pass the flag
+`-index=file=/path/to/index_file` to clangd. *Note that clangd-indexer isn't
   included alongside clangd in the Debian clang-tools package. You will
   likely have to build it from source to use this option.*
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66937: [clangd] Fix the stale documentation about background indexing.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/docs/clangd/Installation.rst:363
 
-- Pass an experimental `-background-index` command line argument.  With
-  this feature enabled, clangd incrementally builds an index of projects
-  that you work on and uses the just-built index automatically.
+- the index is saved to the ``.clangd/index`` in the project root;
+- background indexing canb be disable by the ``--background-index=false`` flag;

kadircet wrote:
> that's not necessarily true, we also save at home directory, and I don't 
> think it is relevant for installation. maybe rather drop it?
I think it is important to mention this. For most cases, .clangd/index is in in 
the project root, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66937



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

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 221510.
usaxena95 marked 5 inline comments as done.
usaxena95 added a comment.

Addressed comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/test/selection-range.test

Index: clang-tools-extra/clangd/test/selection-range.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/selection-range.test
@@ -0,0 +1,39 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void func() {\n}"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/selectionRange","params":{"textDocument":{"uri":"test:///main.cpp"},"positions":[{"line":1,"character":0}]}}
+#  CHECK:  "id": 1
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "parent": {
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 1,
+# CHECK-NEXT:"line": 1
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 0,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 1,
+# CHECK-NEXT:  "line": 1
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 12,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ]
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,7 @@
 # CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
+# CHECK-NEXT:  "selectionRangeProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
 # CHECK-NEXT:  "(",
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -30,6 +30,7 @@
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -416,6 +417,10 @@
   /// textDocument.semanticHighlightingCapabilities.semanticHighlighting
   bool SemanticHighlighting = false;
 
+  /// Client supports semantic selection.
+  /// textDocument.selectionRange
+  bool SemanticSelection = false;
+
   /// Supported encodings for LSP character offsets. (clangd extension).
   llvm::Optional> offsetEncoding;
 
@@ -1222,6 +1227,31 @@
 };
 llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting);
 
+struct SelectionRangeParams {
+  /// The text document.
+  TextDocumentIdentifier textDocument;
+
+  /// The positions inside the text document.
+  std::vector positions;
+};
+bool fromJSON(const llvm::json::Value &, SelectionRangeParams &);
+
+struct SelectionRange {
+  /**
+   * The [range](#Range) of this selection range.
+   */
+  Range range;
+  /**
+   * The parent selection range containing this range. Therefore `parent.range`
+   * must contain `this.range`.
+   */
+  llvm::Optional> parent;
+
+  SelectionRange() = default;
+  SelectionRange(SelectionRange &&) = default;
+};
+llvm::json::Value toJSON(const SelectionRange &);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -15,6 +15,7 @@
 #include "URI.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -335,6 +336,9 @@
   if (auto RenameSupport = Rename->getBoolean("prepareSupport"))
 R.RenamePrepareSupport = *RenameSupport;
 }
+if (auto *SemanticSelection = TextDocument->getObject("selectionRange")) {
+  R.SemanticSelection = t

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 added a comment.

> The SelectionRangeClientCapabilities determines what should the LSP server 
> send the client, if it is true, clangd should send 
> SelectionRangeRegistrationOptions. 
>  But looking at the current specification, it doesn't seem to add too much 
> value. I think we can just simplify return a bool for now (as you did in this 
> patch).

Yeah. So should I remove the client capability since we do not use it and just 
return bool as now?




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1131
+Callback> Reply) {
+  if (Params.positions.size() != 1) {
+elog("{0} positions provided to SelectionRange. Supports exactly one "

hokein wrote:
> usaxena95 wrote:
> > ilya-biryukov wrote:
> > > hokein wrote:
> > > > maybe add an `assert(!Params.positions.empty())`. I think we should not 
> > > > run into this case.
> > > But `Params` comes to clangd over LSP, right?
> > > That means `assert` can fire in case of bad inputs over LSP to clangd.
> > > Bad inputs over LSP should never crash clangd.
> > Yes this comes from the client and can be a bad input. We should just 
> > return error and not crash in such case.
> but the code still doesn't handle the `empty` case?
We do right ?
If the size != 1 then we just return an error.



Comment at: clang-tools-extra/clangd/Protocol.h:1248
+   */
+  llvm::Optional> parent;
+

hokein wrote:
> I think we can simplify the code further, using 
> `llvm::Optional` should be enough, the parent is null for the 
> outer-most range.
I don't think it is possible to do that since the type (SelectionRange) would 
be incomplete at that point. For example size of this class cannot be computed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720



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

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

mostly good, a few nits.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1131
+Callback> Reply) {
+  if (Params.positions.size() != 1) {
+elog("{0} positions provided to SelectionRange. Supports exactly one "

usaxena95 wrote:
> hokein wrote:
> > usaxena95 wrote:
> > > ilya-biryukov wrote:
> > > > hokein wrote:
> > > > > maybe add an `assert(!Params.positions.empty())`. I think we should 
> > > > > not run into this case.
> > > > But `Params` comes to clangd over LSP, right?
> > > > That means `assert` can fire in case of bad inputs over LSP to clangd.
> > > > Bad inputs over LSP should never crash clangd.
> > > Yes this comes from the client and can be a bad input. We should just 
> > > return error and not crash in such case.
> > but the code still doesn't handle the `empty` case?
> We do right ?
> If the size != 1 then we just return an error.
yes, it is correct now. maybe I read the wrong snapshot before.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:136
+SelectionRange render(const std::vector &Ranges) {
+  if (Ranges.empty()) {
+return {};

nit: remove the {}, in LLVM we prefer no {} for a single-statement body.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1169
+Result.emplace_back(render(std::move(*Ranges)));
+return Reply(std::move(Result));
+  });

does `Reply({render(std::move(*Ranges))});` work?



Comment at: clang-tools-extra/clangd/Protocol.h:422
+  /// textDocument.selectionRange
+  bool SemanticSelection = false;
+

I think we could drop this field now, it is not used, as we always return 
`selectionRangeProvider:true`.



Comment at: clang-tools-extra/clangd/Protocol.h:1241
+  /**
+   * The [range](#Range) of this selection range.
+   */

nit: please remove the markdown elements around the `range` in the comment.



Comment at: clang-tools-extra/clangd/Protocol.h:1251
+  SelectionRange() = default;
+  SelectionRange(SelectionRange &&) = default;
+};

Are these constructors needed?



Comment at: clang-tools-extra/clangd/Protocol.h:1248
+   */
+  llvm::Optional> parent;
+

usaxena95 wrote:
> hokein wrote:
> > I think we can simplify the code further, using 
> > `llvm::Optional` should be enough, the parent is null for 
> > the outer-most range.
> I don't think it is possible to do that since the type (SelectionRange) would 
> be incomplete at that point. For example size of this class cannot be 
> computed. 
ah, good point, I see. We could just use `std::unique_ptr 
parent;`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

lebedev.ri wrote:
> This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", "Code 
> Generation Time")` timer.
Thanks, I'm to add it.



Comment at: llvm/trunk/lib/IR/LegacyPassManager.cpp:1686
 
+  llvm::TimeTraceScope TimeScope("OptModule", M.getName());
   for (Function &F : M)

lebedev.ri wrote:
> I think this may be the wrong place for this.
> This includes the entirety of the pipeline, including all of llvm back-end 
> stuff.
Ok, I'm just to delete this line, this block is outputted by 
`MPPassManager::runOnModule()` as well. This is just a section to see module 
name rather than to cover proper part of pipeline.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675



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

[PATCH] D66937: [clangd] Fix the stale documentation about background indexing.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/docs/clangd/Installation.rst:363
 
-- Pass an experimental `-background-index` command line argument.  With
-  this feature enabled, clangd incrementally builds an index of projects
-  that you work on and uses the just-built index automatically.
+- the index is saved to the ``.clangd/index`` in the project root;
+- background indexing canb be disable by the ``--background-index=false`` flag;

hokein wrote:
> kadircet wrote:
> > that's not necessarily true, we also save at home directory, and I don't 
> > think it is relevant for installation. maybe rather drop it?
> I think it is important to mention this. For most cases, .clangd/index is in 
> in the project root, right?
yes, but still if you are planning to mention this please also mention 
something like: `Index shards for common headers like standard library will be 
stored in $HOME/.clangd/index`



Comment at: clang-tools-extra/docs/clangd/Installation.rst:365
+- background indexing canb be disable by the ``--background-index=false`` flag;
+  if it is disabled, clangd doesn't have a global view of the whole project, it
+  only has a view on symbols coming from files you are currently editing;

kadircet wrote:
> instead of `if it's disabled...`
> 
> ```
> Note that, disabling background-index will limit clangd's knowledge about 
> your codebase to
> files you are currently editing.
> ```
seems to be the same?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66937



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

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 221521.
usaxena95 marked 9 inline comments as done.
usaxena95 added a comment.

Addressed comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/test/selection-range.test

Index: clang-tools-extra/clangd/test/selection-range.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/selection-range.test
@@ -0,0 +1,39 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void func() {\n}"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/selectionRange","params":{"textDocument":{"uri":"test:///main.cpp"},"positions":[{"line":1,"character":0}]}}
+#  CHECK:  "id": 1
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "parent": {
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 1,
+# CHECK-NEXT:"line": 1
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 0,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 1,
+# CHECK-NEXT:  "line": 1
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 12,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ]
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,7 @@
 # CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
+# CHECK-NEXT:  "selectionRangeProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
 # CHECK-NEXT:  "(",
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -30,6 +30,7 @@
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -1222,6 +1223,28 @@
 };
 llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting);
 
+struct SelectionRangeParams {
+  /// The text document.
+  TextDocumentIdentifier textDocument;
+
+  /// The positions inside the text document.
+  std::vector positions;
+};
+bool fromJSON(const llvm::json::Value &, SelectionRangeParams &);
+
+struct SelectionRange {
+  /**
+   * The range of this selection range.
+   */
+  Range range;
+  /**
+   * The parent selection range containing this range. Therefore `parent.range`
+   * must contain `this.range`.
+   */
+  std::unique_ptr parent;
+};
+llvm::json::Value toJSON(const SelectionRange &);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -15,6 +15,7 @@
 #include "URI.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -1073,5 +1074,18 @@
   };
 }
 
+bool fromJSON(const llvm::json::Value &Params, SelectionRangeParams &P) {
+  llvm::json::ObjectMapper O(Params);
+  return O && O.map("textDocument", P.textDocument) &&
+ O.map("positions", P.positions);
+}
+
+llvm::json::Value toJSON(const SelectionRange &Out) {
+  if (Out.parent) {
+return llvm::json::Object{{"range", Out.range},
+  {"parent", toJSON(*Out.parent)}};
+  }
+  return llvm::json::Object{{"range", Out.range}};
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPSe

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1169
+Result.emplace_back(render(std::move(*Ranges)));
+return Reply(std::move(Result));
+  });

hokein wrote:
> does `Reply({render(std::move(*Ranges))});` work?
List initialization of vector of move only types is painful 😃 
Even this does not work: 
`Reply(std::vector{render(std::move(*Ranges))});` because I 
think it vector tries to copy construct it because of initializer_lists 



Comment at: clang-tools-extra/clangd/Protocol.h:1251
+  SelectionRange() = default;
+  SelectionRange(SelectionRange &&) = default;
+};

hokein wrote:
> Are these constructors needed?
We don't need them if we use just unique_ptr. They were needed before in 
Optional. Somehow it was not able to deduce this is a 
trivially-constructible + move only class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720



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

[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 9 inline comments as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/FindTarget.cpp:432
+  Ref = ReferenceLoc{
+  E->getQualifierLoc(), E->getNameInfo().getLoc(), {E->getDecl()}};
+}

kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > I believe it is better to return `getFoundDecl` then `getDecl`, the 
> > > former respects using declarations.
> > Good point. Done. Added a test too.
> I was actually referring to `DeclRefExpr`, change seems to be on `MemberExpr`
Good point. Fixed that one too, thanks!



Comment at: clang-tools-extra/clangd/FindTarget.cpp:548
+  bool TraverseElaboratedTypeLoc(ElaboratedTypeLoc L) {
+// ElaboratedTypeLoc will reports information for its inner type loc.
+// Otherwise we loose information about inner types loc's qualifier.

kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > why not just traversenestednamespecifier and `visitNode(L)` instead of 
> > > calling the base::traverse ?
> > To avoid re-implementing the traversal logic. `Base::Traverse` does exactly 
> > that, we shouldn't re-implement traversal ourselves.
> I see but that should help get rid of `TypeLocsToSkip` and some extra 
> traversals. I believe it would be worth getting rid of the additional state 
> management, but up to you.
I like the idea and actually tried to implement it before, but failed. The 
nested-name-specifier case is simple and we could actually implement it.

However, the `ElaboratedTypeLoc` case is more complicated: we have to call 
`Base::TraverseTypeLoc` to make sure we recursively visit children of 
`ElaboratedTypeLoc::getNamedType` (e.g. template arguments inside the typeloc). 
However, `Base::TraverseTypeLoc` automatically calls `VisitTypeLoc` and our 
qualifier is forgotten again.

Let me know if there's a way to implement this that I missed, though.



Comment at: clang-tools-extra/clangd/FindTarget.cpp:577
+  /// (!) For the purposes of this function declarations are not considered to
+  /// be references. However, declarations can have:wa references inside
+  /// them, e.g. 'namespace foo = std' references namespace 'std' and this

kadircet wrote:
> `:wa` is still around `have:wa references` :D
Thanks. This slipped my attention last time :-)



Comment at: clang-tools-extra/clangd/FindTarget.cpp:609
+// FIXME: should this be done by 'explicitReference'?
+if (Ref->NameLoc.isInvalid() || Ref->NameLoc.isMacroID())
+  return;

kadircet wrote:
> I can see the second check is for getting rid of references coming from macro 
> expansions. But what exactly is the first one for? How can we get an invalid 
> nameloc, could you also add it as a comment?
Ah, thanks for spotting this. I don't think we want to filter out macro 
references, fixed that and added a test

The first one is a precaution against various implicit nodes getting into the 
traversal. Normally this shouldn't happen, unless:
- RecursiveASTVisitor has bugs and visits implicit nodes (e.g. generated 
begin() calls for range-based-for )
- Users start traversal from an implicitly-generated statement.

I couldn't come up with examples of that actually happening, but I'm also 
scared of adding an assertion as I feel this would inevitably fail at some 
point.
At the same time, the whole point of this function is to only expose those 
references that could be meaningfully found in the source code (e.g. for 
refactoring and syntax highlighting), i.e. they should always have (at least) 
source location of the name.

Added a comment, let me know if this could be made clearer, though.



Comment at: clang-tools-extra/clangd/FindTarget.h:96
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ReferenceLoc R);
+

kadircet wrote:
> Is this for testing purposes? maybe move it into `findtargettests.cpp` or 
> make it a member helper like `Print(SourceManager&)` so that it can also 
> print locations etc.?
Yes, this is for output in the test. Moving to `findTargetTests.cpp`, we risk 
ODR violations in the future.
In any case, it's useful to have debug output and `operator<<` is common for 
that purpose: it enables `llvm::toString` and, consecutively, 
`llvm::formatv("{0}", R)`.

What are the downsides of having it?




Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:498
+std::string AnnotatedCode;
+unsigned NextOffset = 0;
+for (unsigned I = 0; I < Refs.size(); ++I) {

kadircet wrote:
> this sounds more like `LastOffset` or `PrevOffset`
That's the next character in `Code` we need to process, renamed to 
`NextCodeChar` to specifically mention what string we're referring to.



Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:504
+  assert(Pos.isValid());
+  if

[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 221522.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.

- Use DeclRefExpr::getFoundDecl, add a test
- Update a comment
- Remove redundant anon namespace declaration
- Remove :wa
- Do not filter-out macro references
- Document why we drop references with invalid locations, log if that happens
- Rename NextOffset to NextCodeChar


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/FindTarget.h
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -9,6 +9,9 @@
 
 #include "Selection.h"
 #include "TestTU.h"
+#include "clang/AST/Decl.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/Annotations.h"
 #include "gmock/gmock.h"
@@ -464,6 +467,223 @@
   EXPECT_DECLS("ObjCObjectTypeLoc");
 }
 
+class FindExplicitReferencesTest : public ::testing::Test {
+protected:
+  struct AllRefs {
+std::string AnnotatedCode;
+std::string DumpedReferences;
+  };
+
+  /// Parses \p Code, finds function '::foo' and annotates its body with results
+  /// of findExplicitReferecnces.
+  /// See actual tests for examples of annotation format.
+  AllRefs annotateReferencesInFoo(llvm::StringRef Code) {
+TestTU TU;
+TU.Code = Code;
+
+auto AST = TU.build();
+auto &Func = llvm::cast(findDecl(AST, "foo"));
+
+std::vector Refs;
+findExplicitReferences(Func.getBody(), [&Refs](ReferenceLoc R) {
+  Refs.push_back(std::move(R));
+});
+
+auto &SM = AST.getSourceManager();
+llvm::sort(Refs, [&](const ReferenceLoc &L, const ReferenceLoc &R) {
+  return SM.isBeforeInTranslationUnit(L.NameLoc, R.NameLoc);
+});
+
+std::string AnnotatedCode;
+unsigned NextCodeChar = 0;
+for (unsigned I = 0; I < Refs.size(); ++I) {
+  auto &R = Refs[I];
+
+  SourceLocation Pos = R.NameLoc;
+  assert(Pos.isValid());
+  if (Pos.isMacroID()) // FIXME: figure out how to show macro locations.
+Pos = SM.getExpansionLoc(Pos);
+  assert(Pos.isFileID());
+
+  FileID File;
+  unsigned Offset;
+  std::tie(File, Offset) = SM.getDecomposedLoc(Pos);
+  if (File == SM.getMainFileID()) {
+// Print the reference in a source code.
+assert(NextCodeChar <= Offset);
+AnnotatedCode += Code.substr(NextCodeChar, Offset - NextCodeChar);
+AnnotatedCode += "$" + std::to_string(I) + "^";
+
+NextCodeChar = Offset;
+  }
+}
+AnnotatedCode += Code.substr(NextCodeChar);
+
+std::string DumpedReferences;
+for (unsigned I = 0; I < Refs.size(); ++I)
+  DumpedReferences += llvm::formatv("{0}: {1}\n", I, Refs[I]);
+
+return AllRefs{std::move(AnnotatedCode), std::move(DumpedReferences)};
+  }
+};
+
+TEST_F(FindExplicitReferencesTest, All) {
+  std::pair Cases[] =
+  {
+  // Simple expressions.
+  {R"cpp(
+int global;
+int func();
+void foo(int param) {
+  $0^global = $1^param + $2^func();
+}
+)cpp",
+   "0: targets = {global}\n"
+   "1: targets = {param}\n"
+   "2: targets = {func}\n"},
+  {R"cpp(
+struct X { int a; };
+void foo(X x) {
+  $0^x.$1^a = 10;
+}
+)cpp",
+   "0: targets = {x}\n"
+   "1: targets = {X::a}\n"},
+  // Namespaces and aliases.
+  {R"cpp(
+  namespace ns {}
+  namespace alias = ns;
+  void foo() {
+using namespace $0^ns;
+using namespace $1^alias;
+  }
+)cpp",
+   "0: targets = {ns}\n"
+   "1: targets = {alias}\n"},
+  // Using declarations.
+  {R"cpp(
+  namespace ns { int global; }
+  void foo() {
+using $0^ns::$1^global;
+  }
+)cpp",
+   "0: targets = {ns}\n"
+   "1: targets = {ns::global}, qualifier = 'ns::'\n"},
+  // Simple types.
+  {R"cpp(
+ struct Struct { int a; };
+ using Typedef = int;
+ void foo() {
+   $0^Struct x;
+   $1^Typedef y;
+   static_cast<$2^Struct*>(0);
+ }
+   )cpp",
+   "0: targets = {Struct}\n"
+   "1: targets = {Typedef}\n"
+   "2: targets = {Struct}\n"},
+  // Name qualifiers.
+  {R"cpp(
+ namespace a { namespace b { struct S { typedef int type; }; } }
+ void foo() {
+   $0^a::$1^b::$2^S x;
+   using namespace $3^a::$4^b;
+   $5^

[PATCH] D66937: [clangd] Fix the stale documentation about background indexing.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 221523.
hokein marked 2 inline comments as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66937

Files:
  clang-tools-extra/docs/clangd/Installation.rst


Index: clang-tools-extra/docs/clangd/Installation.rst
===
--- clang-tools-extra/docs/clangd/Installation.rst
+++ clang-tools-extra/docs/clangd/Installation.rst
@@ -352,20 +352,29 @@
 Creating this file by hand is a reasonable place to start if your project is
 quite simple.
 
-Project-wide Index
-==
+Background Indexing
+===
 
-By default clangd only has a view on symbols coming from files you are
-currently editing. You can extend this view to whole project by providing a
-project-wide index to clangd.  There are two ways to do this.
+clangd builds an incremental index of your project (all files listed in the
+compilation database). The index improves code navigation features 
(go-to-definition,
+find-references) and code completion.
 
-- Pass an experimental `-background-index` command line argument.  With
-  this feature enabled, clangd incrementally builds an index of projects
-  that you work on and uses the just-built index automatically.
+- clangd only uses idle cores to build the index, you can limit the total
+  amount of cores by passing the `-j=` flag;
+- the index is saved to the ``.clangd/index`` in the project root; index 
shareds
+  for common headers e.g. STL will be stored in `$HOME/.clangd/index`;
+- background indexing can be disabled by the ``--background-index=false`` flag;
+  Note that, disabling background-index will limit clangd's knowledge about 
your
+  codebase to files you are currently editing.
 
-- Generate an index file using `clangd-indexer
+Build Index Manually
+
+
+**DISCLAIMER: This is mainly for clangd developers.**
+
+There is a `clangd-indexer
   
`__
-  Then you can pass generated index file to clangd using
-  `-index-file=/path/to/index_file`.  *Note that clangd-indexer isn't
+which generates an index file for your project. To use the index, pass the flag
+`-index=file=/path/to/index_file` to clangd. *Note that clangd-indexer isn't
   included alongside clangd in the Debian clang-tools package. You will
   likely have to build it from source to use this option.*


Index: clang-tools-extra/docs/clangd/Installation.rst
===
--- clang-tools-extra/docs/clangd/Installation.rst
+++ clang-tools-extra/docs/clangd/Installation.rst
@@ -352,20 +352,29 @@
 Creating this file by hand is a reasonable place to start if your project is
 quite simple.
 
-Project-wide Index
-==
+Background Indexing
+===
 
-By default clangd only has a view on symbols coming from files you are
-currently editing. You can extend this view to whole project by providing a
-project-wide index to clangd.  There are two ways to do this.
+clangd builds an incremental index of your project (all files listed in the
+compilation database). The index improves code navigation features (go-to-definition,
+find-references) and code completion.
 
-- Pass an experimental `-background-index` command line argument.  With
-  this feature enabled, clangd incrementally builds an index of projects
-  that you work on and uses the just-built index automatically.
+- clangd only uses idle cores to build the index, you can limit the total
+  amount of cores by passing the `-j=` flag;
+- the index is saved to the ``.clangd/index`` in the project root; index shareds
+  for common headers e.g. STL will be stored in `$HOME/.clangd/index`;
+- background indexing can be disabled by the ``--background-index=false`` flag;
+  Note that, disabling background-index will limit clangd's knowledge about your
+  codebase to files you are currently editing.
 
-- Generate an index file using `clangd-indexer
+Build Index Manually
+
+
+**DISCLAIMER: This is mainly for clangd developers.**
+
+There is a `clangd-indexer
   `__
-  Then you can pass generated index file to clangd using
-  `-index-file=/path/to/index_file`.  *Note that clangd-indexer isn't
+which generates an index file for your project. To use the index, pass the flag
+`-index=file=/path/to/index_file` to clangd. *Note that clangd-indexer isn't
   included alongside clangd in the Debian clang-tools package. You will
   likely have to build it from source to use this option.*
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66937: [clangd] Fix the stale documentation about background indexing.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/docs/clangd/Installation.rst:365
+- background indexing canb be disable by the ``--background-index=false`` flag;
+  if it is disabled, clangd doesn't have a global view of the whole project, it
+  only has a view on symbols coming from files you are currently editing;

kadircet wrote:
> kadircet wrote:
> > instead of `if it's disabled...`
> > 
> > ```
> > Note that, disabling background-index will limit clangd's knowledge about 
> > your codebase to
> > files you are currently editing.
> > ```
> seems to be the same?
oops, I forgot this comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66937



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

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[Haojian Wu via Phabricator via cfe-commits]

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

thanks, looks good.




Comment at: clang-tools-extra/clangd/Protocol.cpp:18
 #include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"

the header seems not used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720



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

[PATCH] D67877: [analyzer] Conditionnaly include clang Analysis examples with cmake.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a subscriber: tstellar.
lebedev.ri added a comment.

Actually, uh oh. @Jiboo can you please file this as a bug?
This really should go into next patch release. CC @tstellar

@Szelethus this breaks LLVMExports.cmake as compared with LLVM-8, so it's 
actually a critical bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67877



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

r372749 - [Diagnostics] Handle tautological left shifts in boolean context

[David Bolvansky via cfe-commits]

Author: xbolva00
Date: Tue Sep 24 06:14:18 2019
New Revision: 372749

URL: http://llvm.org/viewvc/llvm-project?rev=372749&view=rev
Log:
[Diagnostics] Handle tautological left shifts in boolean context 


Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/warn-int-in-bool-context.c

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=372749&r1=372748&r2=372749&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Sep 24 06:14:18 
2019
@@ -6155,6 +6155,10 @@ def warn_integer_constants_in_conditiona
   "converting the result of '?:' with integer constants to a boolean always "
   "evaluates to 'true'">,
   InGroup;
+def warn_left_shift_always : Warning<
+  "converting the result of '<<' to a boolean always evaluates "
+  "to %select{false|true}0">,
+  InGroup;
 def warn_comparison_of_mixed_enum_types : Warning<
   "comparison of two values with different enumeration types"
   "%diff{ ($ and $)|}0,1">,

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=372749&r1=372748&r2=372749&view=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue Sep 24 06:14:18 2019
@@ -11314,17 +11314,26 @@ static void DiagnoseIntInBoolContext(Sem
 
   if (const auto *BO = dyn_cast(E)) {
 BinaryOperator::Opcode Opc = BO->getOpcode();
+Expr::EvalResult Result;
 // Do not diagnose unsigned shifts.
-if (Opc == BO_Shl && E->getType()->isSignedIntegerType())
-  S.Diag(ExprLoc, diag::warn_left_shift_in_bool_context) << E;
+if (Opc == BO_Shl) {
+  const auto *LHS = getIntegerLiteral(BO->getLHS());
+  const auto *RHS = getIntegerLiteral(BO->getRHS());
+  if (LHS && LHS->getValue() == 0)
+S.Diag(ExprLoc, diag::warn_left_shift_always) << 0;
+  else if (RHS && RHS->getValue().isNonNegative() &&
+   E->EvaluateAsInt(Result, S.Context, Expr::SE_AllowSideEffects))
+S.Diag(ExprLoc, diag::warn_left_shift_always)
+<< (Result.Val.getInt() != 0);
+  else if (E->getType()->isSignedIntegerType())
+S.Diag(ExprLoc, diag::warn_left_shift_in_bool_context) << E;
+}
   }
 
   if (const auto *CO = dyn_cast(E)) {
 const auto *LHS = getIntegerLiteral(CO->getTrueExpr());
-if (!LHS)
-  return;
 const auto *RHS = getIntegerLiteral(CO->getFalseExpr());
-if (!RHS)
+if (!LHS || !RHS)
   return;
 if ((LHS->getValue() == 0 || LHS->getValue() == 1) &&
 (RHS->getValue() == 0 || RHS->getValue() == 1))

Modified: cfe/trunk/test/Sema/warn-int-in-bool-context.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-int-in-bool-context.c?rev=372749&r1=372748&r2=372749&view=diff
==
--- cfe/trunk/test/Sema/warn-int-in-bool-context.c (original)
+++ cfe/trunk/test/Sema/warn-int-in-bool-context.c Tue Sep 24 06:14:18 2019
@@ -24,12 +24,17 @@ enum num {
 
 int test(int a, unsigned b, enum num n) {
   boolean r;
-  r = a << a; // expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(a << a) != 0'?}}
-  r = MM; // expected-warning {{converting the result of '<<' to a boolean; 
did you mean '(a << a) != 0'?}}
-  r = (1 << 7); // expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(1 << 7) != 0'?}}
-  r = 2UL << 2;
-  r = 2 << b; // expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(2 << b) != 0'?}}
-  r = (unsigned)(2 << b); 
+  r = a << a;// expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(a << a) != 0'?}}
+  r = MM;// expected-warning {{converting the result of '<<' to a 
boolean; did you mean '(a << a) != 0'?}}
+  r = (1 << 7);  // expected-warning {{converting the result of '<<' to a 
boolean always evaluates to true}}
+  r = 2UL << 2;  // expected-warning {{converting the result of '<<' to a 
boolean always evaluates to true}}
+  r = 0 << a;// expected-warning {{converting the result of '<<' to a 
boolean always evaluates to false}}
+  r = 0 << 2;// expected-warning {{converting the result of '<<' to a 
boolean always evaluates to false}}
+  r = 1 << 0;// expected-warning {{converting the result of '<<' to a 
boolean always evaluates to true}}
+  r = 1 << 2;// expected-warning {{converting the result of '<<' to a 
boolean always evaluates to true}}
+  r = 1ULL << 2; // expected-warning {{converting the result of '<<' to a 
boolean always evaluates to true}}
+  r = 2 << b;// expec

[PATCH] D67877: [analyzer] Conditionnaly include clang Analysis examples with cmake.

[Jean-Bapiste Lepesme via Phabricator via cfe-commits]

Jiboo added a comment.

Here you go: https://bugs.llvm.org/show_bug.cgi?id=43430


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67877



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

[PATCH] D67960: [clangd] Move the existing heder-source-switch implemenation out of clangdServer.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.

This is a NFC change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67960

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/HeaderSourceSwitch.cpp
  clang-tools-extra/clangd/HeaderSourceSwitch.h
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp

Index: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
+++ clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp
@@ -25,6 +25,60 @@
   return false;
 }
 
+TEST(HeaderSourceSwitchTest, FileHeuristic) {
+  MockFSProvider FS;
+  auto FooCpp = testPath("foo.cpp");
+  auto FooH = testPath("foo.h");
+  auto Invalid = testPath("main.cpp");
+
+  FS.Files[FooCpp];
+  FS.Files[FooH];
+  FS.Files[Invalid];
+  Optional PathResult =
+  getCorrespondingHeaderOrSource(FooCpp, FS.getFileSystem());
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), FooH);
+
+  PathResult = getCorrespondingHeaderOrSource(FooH, FS.getFileSystem());
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), FooCpp);
+
+  // Test with header file in capital letters and different extension, source
+  // file with different extension
+  auto FooC = testPath("bar.c");
+  auto FooHH = testPath("bar.HH");
+
+  FS.Files[FooC];
+  FS.Files[FooHH];
+  PathResult = getCorrespondingHeaderOrSource(FooC, FS.getFileSystem());
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), FooHH);
+
+  // Test with both capital letters
+  auto Foo2C = testPath("foo2.C");
+  auto Foo2HH = testPath("foo2.HH");
+  FS.Files[Foo2C];
+  FS.Files[Foo2HH];
+  PathResult = getCorrespondingHeaderOrSource(Foo2C, FS.getFileSystem());
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), Foo2HH);
+
+  // Test with source file as capital letter and .hxx header file
+  auto Foo3C = testPath("foo3.C");
+  auto Foo3HXX = testPath("foo3.hxx");
+
+  FS.Files[Foo3C];
+  FS.Files[Foo3HXX];
+  PathResult = getCorrespondingHeaderOrSource(Foo3C, FS.getFileSystem());
+  EXPECT_TRUE(PathResult.hasValue());
+  ASSERT_EQ(PathResult.getValue(), Foo3HXX);
+
+  // Test if asking for a corresponding file that doesn't exist returns an empty
+  // string.
+  PathResult = getCorrespondingHeaderOrSource(Invalid, FS.getFileSystem());
+  EXPECT_FALSE(PathResult.hasValue());
+}
+
 TEST(HeaderSourceSwitchTest, GetLocalDecls) {
   TestTU TU;
   TU.HeaderCode = R"cpp(
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -765,82 +765,6 @@
   }
 }
 
-TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
-  MockFSProvider FS;
-  ErrorCheckingDiagConsumer DiagConsumer;
-  MockCompilationDatabase CDB;
-  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
-
-  auto SourceContents = R"cpp(
-  #include "foo.h"
-  int b = a;
-  )cpp";
-
-  auto FooCpp = testPath("foo.cpp");
-  auto FooH = testPath("foo.h");
-  auto Invalid = testPath("main.cpp");
-
-  FS.Files[FooCpp] = SourceContents;
-  FS.Files[FooH] = "int a;";
-  FS.Files[Invalid] = "int main() { \n return 0; \n }";
-
-  Optional PathResult = Server.switchSourceHeader(FooCpp);
-  EXPECT_TRUE(PathResult.hasValue());
-  ASSERT_EQ(PathResult.getValue(), FooH);
-
-  PathResult = Server.switchSourceHeader(FooH);
-  EXPECT_TRUE(PathResult.hasValue());
-  ASSERT_EQ(PathResult.getValue(), FooCpp);
-
-  SourceContents = R"c(
-  #include "foo.HH"
-  int b = a;
-  )c";
-
-  // Test with header file in capital letters and different extension, source
-  // file with different extension
-  auto FooC = testPath("bar.c");
-  auto FooHH = testPath("bar.HH");
-
-  FS.Files[FooC] = SourceContents;
-  FS.Files[FooHH] = "int a;";
-
-  PathResult = Server.switchSourceHeader(FooC);
-  EXPECT_TRUE(PathResult.hasValue());
-  ASSERT_EQ(PathResult.getValue(), FooHH);
-
-  // Test with both capital letters
-  auto Foo2C = testPath("foo2.C");
-  auto Foo2HH = testPath("foo2.HH");
-  FS.Files[Foo2C] = SourceContents;
-  FS.Files[Foo2HH] = "int a;";
-
-  PathResult = Server.switchSourceHeader(Foo2C);
-  EXPECT_TRUE(PathResult.hasValue());
-  ASSERT_EQ(PathResult.getValue(), Foo2HH);
-
-  // Test with source file as capital letter and .hxx header file
-  auto Foo3C = testPath("foo3.C");
-  auto Foo3HXX = testPath("foo3.hxx");
-
-  SourceContents = R"c(
-  #include "foo3.hxx"
-  int b = a;
-  )c";
-
-  FS.Files[Foo3C] = SourceContents;
-  FS.Files[Foo3HXX] = "int a;";
-
-  PathResult = Server.switchSourceHeader(Foo3C);
-  EX

[clang-tools-extra] r372752 - [clangd] Save an unnecessary copy, NFC.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Tue Sep 24 06:25:38 2019
New Revision: 372752

URL: http://llvm.org/viewvc/llvm-project?rev=372752&view=rev
Log:
[clangd] Save an unnecessary copy, NFC.

Modified:
clang-tools-extra/trunk/clangd/FileDistance.cpp

Modified: clang-tools-extra/trunk/clangd/FileDistance.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FileDistance.cpp?rev=372752&r1=372751&r2=372752&view=diff
==
--- clang-tools-extra/trunk/clangd/FileDistance.cpp (original)
+++ clang-tools-extra/trunk/clangd/FileDistance.cpp Tue Sep 24 06:25:38 2019
@@ -208,7 +208,7 @@ createScopeFileDistance(llvm::ArrayRef QueryScopes)


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

[PATCH] D67961: [libTooling] Introduce the MatchConsumer abstraction

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a project: clang.

This revision introduces a separate (small) library for the `MatchConsumer`
abstraction: computations over AST match results.  This abstraction is central
to the Transformer framework, and there deserves being defined explicitly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67961

Files:
  clang/include/clang/Tooling/Refactoring/MatchConsumer.h
  clang/include/clang/Tooling/Refactoring/RangeSelector.h
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/RangeSelector.cpp

Index: clang/lib/Tooling/Refactoring/RangeSelector.cpp
===
--- clang/lib/Tooling/Refactoring/RangeSelector.cpp
+++ clang/lib/Tooling/Refactoring/RangeSelector.cpp
@@ -310,11 +310,3 @@
 return Result.SourceManager->getExpansionRange(*SRange);
   };
 }
-
-RangeSelector tooling::ifBound(std::string ID, RangeSelector TrueSelector,
-   RangeSelector FalseSelector) {
-  return [=](const MatchResult &Result) {
-auto &Map = Result.Nodes.getMap();
-return (Map.find(ID) != Map.end() ? TrueSelector : FalseSelector)(Result);
-  };
-}
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -19,6 +19,7 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchersInternal.h"
 #include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Refactoring/MatchConsumer.h"
 #include "clang/Tooling/Refactoring/RangeSelector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
@@ -32,11 +33,7 @@
 namespace clang {
 namespace tooling {
 
-// Note that \p TextGenerator is allowed to fail, e.g. when trying to access a
-// matched node that was not bound.  Allowing this to fail simplifies error
-// handling for interactive tools like clang-query.
-using TextGenerator = std::function(
-const ast_matchers::MatchFinder::MatchResult &)>;
+using TextGenerator = MatchConsumer;
 
 /// Wraps a string as a TextGenerator.
 inline TextGenerator text(std::string M) {
Index: clang/include/clang/Tooling/Refactoring/RangeSelector.h
===
--- clang/include/clang/Tooling/Refactoring/RangeSelector.h
+++ clang/include/clang/Tooling/Refactoring/RangeSelector.h
@@ -17,14 +17,14 @@
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Refactoring/MatchConsumer.h"
 #include "llvm/Support/Error.h"
 #include 
 #include 
 
 namespace clang {
 namespace tooling {
-using RangeSelector = std::function(
-const ast_matchers::MatchFinder::MatchResult &)>;
+using RangeSelector = MatchConsumer;
 
 inline RangeSelector charRange(CharSourceRange R) {
   return [R](const ast_matchers::MatchFinder::MatchResult &)
@@ -87,11 +87,6 @@
 /// source), if `S` is an expansion, and `S` itself, otherwise.  Corresponds to
 /// `SourceManager::getExpansionRange`.
 RangeSelector expansion(RangeSelector S);
-
-/// Chooses between the two selectors, based on whether \p ID is bound in the
-/// match.
-RangeSelector ifBound(std::string ID, RangeSelector TrueSelector,
-  RangeSelector FalseSelector);
 } // namespace tooling
 } // namespace clang
 
Index: clang/include/clang/Tooling/Refactoring/MatchConsumer.h
===
--- /dev/null
+++ clang/include/clang/Tooling/Refactoring/MatchConsumer.h
@@ -0,0 +1,58 @@
+//===--- MatchConsumer.h - MatchConsumer abstraction *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// /file This file defines the *MatchConsumer* abstraction: a computation over
+/// match results, specifically the `ast_matchers::MatchFinder::MatchResult`
+/// class.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLING_REFACTOR_MATCH_CONSUMER_H_
+#define LLVM_CLANG_TOOLING_REFACTOR_MATCH_CONSUMER_H_
+
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace tooling {
+
+/// A central abstraction of the Transformer framework is computation over the
+/// results of a match (represented by \c MatchFinder::MatchResult). We
+/// standardize this abstraction with the \c MatchConsumer type.  Since match
+/// results inclu

[PATCH] D67961: [libTooling] Introduce the MatchConsumer abstraction

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 221531.
ymandel added a comment.

clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67961

Files:
  clang/include/clang/Tooling/Refactoring/MatchConsumer.h
  clang/include/clang/Tooling/Refactoring/RangeSelector.h
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/RangeSelector.cpp

Index: clang/lib/Tooling/Refactoring/RangeSelector.cpp
===
--- clang/lib/Tooling/Refactoring/RangeSelector.cpp
+++ clang/lib/Tooling/Refactoring/RangeSelector.cpp
@@ -310,11 +310,3 @@
 return Result.SourceManager->getExpansionRange(*SRange);
   };
 }
-
-RangeSelector tooling::ifBound(std::string ID, RangeSelector TrueSelector,
-   RangeSelector FalseSelector) {
-  return [=](const MatchResult &Result) {
-auto &Map = Result.Nodes.getMap();
-return (Map.find(ID) != Map.end() ? TrueSelector : FalseSelector)(Result);
-  };
-}
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -19,6 +19,7 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchersInternal.h"
 #include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Refactoring/MatchConsumer.h"
 #include "clang/Tooling/Refactoring/RangeSelector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
@@ -32,11 +33,7 @@
 namespace clang {
 namespace tooling {
 
-// Note that \p TextGenerator is allowed to fail, e.g. when trying to access a
-// matched node that was not bound.  Allowing this to fail simplifies error
-// handling for interactive tools like clang-query.
-using TextGenerator = std::function(
-const ast_matchers::MatchFinder::MatchResult &)>;
+using TextGenerator = MatchConsumer;
 
 /// Wraps a string as a TextGenerator.
 inline TextGenerator text(std::string M) {
Index: clang/include/clang/Tooling/Refactoring/RangeSelector.h
===
--- clang/include/clang/Tooling/Refactoring/RangeSelector.h
+++ clang/include/clang/Tooling/Refactoring/RangeSelector.h
@@ -17,14 +17,14 @@
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Refactoring/MatchConsumer.h"
 #include "llvm/Support/Error.h"
 #include 
 #include 
 
 namespace clang {
 namespace tooling {
-using RangeSelector = std::function(
-const ast_matchers::MatchFinder::MatchResult &)>;
+using RangeSelector = MatchConsumer;
 
 inline RangeSelector charRange(CharSourceRange R) {
   return [R](const ast_matchers::MatchFinder::MatchResult &)
@@ -87,11 +87,6 @@
 /// source), if `S` is an expansion, and `S` itself, otherwise.  Corresponds to
 /// `SourceManager::getExpansionRange`.
 RangeSelector expansion(RangeSelector S);
-
-/// Chooses between the two selectors, based on whether \p ID is bound in the
-/// match.
-RangeSelector ifBound(std::string ID, RangeSelector TrueSelector,
-  RangeSelector FalseSelector);
 } // namespace tooling
 } // namespace clang
 
Index: clang/include/clang/Tooling/Refactoring/MatchConsumer.h
===
--- /dev/null
+++ clang/include/clang/Tooling/Refactoring/MatchConsumer.h
@@ -0,0 +1,58 @@
+//===--- MatchConsumer.h - MatchConsumer abstraction *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// /file This file defines the *MatchConsumer* abstraction: a computation over
+/// match results, specifically the `ast_matchers::MatchFinder::MatchResult`
+/// class.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLING_REFACTOR_MATCH_CONSUMER_H_
+#define LLVM_CLANG_TOOLING_REFACTOR_MATCH_CONSUMER_H_
+
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace tooling {
+
+/// A central abstraction of the Transformer framework is computation over the
+/// results of a match (represented by \c MatchFinder::MatchResult). We
+/// standardize this abstraction with the \c MatchConsumer type.  Since match
+/// results include dynamically-bound variables, these computations are allowed
+/// to fail, for example when trying to access a matched node that was not
+/// bound. Allowing for failure in the 

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 221534.
usaxena95 added a comment.

Removed ununsed header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67720

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/test/selection-range.test

Index: clang-tools-extra/clangd/test/selection-range.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/selection-range.test
@@ -0,0 +1,39 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void func() {\n}"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/selectionRange","params":{"textDocument":{"uri":"test:///main.cpp"},"positions":[{"line":1,"character":0}]}}
+#  CHECK:  "id": 1
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "parent": {
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 1,
+# CHECK-NEXT:"line": 1
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 0,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 1,
+# CHECK-NEXT:  "line": 1
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 12,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ]
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -33,6 +33,7 @@
 # CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
+# CHECK-NEXT:  "selectionRangeProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
 # CHECK-NEXT:  "(",
Index: clang-tools-extra/clangd/Protocol.h
===
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -30,6 +30,7 @@
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -1222,6 +1223,28 @@
 };
 llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting);
 
+struct SelectionRangeParams {
+  /// The text document.
+  TextDocumentIdentifier textDocument;
+
+  /// The positions inside the text document.
+  std::vector positions;
+};
+bool fromJSON(const llvm::json::Value &, SelectionRangeParams &);
+
+struct SelectionRange {
+  /**
+   * The range of this selection range.
+   */
+  Range range;
+  /**
+   * The parent selection range containing this range. Therefore `parent.range`
+   * must contain `this.range`.
+   */
+  std::unique_ptr parent;
+};
+llvm::json::Value toJSON(const SelectionRange &);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/clangd/Protocol.cpp
===
--- clang-tools-extra/clangd/Protocol.cpp
+++ clang-tools-extra/clangd/Protocol.cpp
@@ -1073,5 +1073,18 @@
   };
 }
 
+bool fromJSON(const llvm::json::Value &Params, SelectionRangeParams &P) {
+  llvm::json::ObjectMapper O(Params);
+  return O && O.map("textDocument", P.textDocument) &&
+ O.map("positions", P.positions);
+}
+
+llvm::json::Value toJSON(const SelectionRange &Out) {
+  if (Out.parent) {
+return llvm::json::Object{{"range", Out.range},
+  {"parent", toJSON(*Out.parent)}};
+  }
+  return llvm::json::Object{{"range", Out.range}};
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -107,6 +107,8 @@
   void onChangeConfiguration(const DidChangeConfigurationParams &);
   void onSymbolInfo(const TextDocumentPositionParams &,
 Callback>);
+  void onSelectionRange(const SelectionRangeParam

[clang-tools-extra] r372753 - [clangd] Add semantic selection to ClangdLSPServer.

[Utkarsh Saxena via cfe-commits]

Author: usaxena95
Date: Tue Sep 24 06:38:33 2019
New Revision: 372753

URL: http://llvm.org/viewvc/llvm-project?rev=372753&view=rev
Log:
[clangd] Add semantic selection to ClangdLSPServer.

Summary:
This adds semantic selection to the LSP Server.
Adds support for serialization of input request and the output reply.
Also adds regression tests for the feature.

Currently we do not support multi cursor.The LSP Server only accepts single 
position in the request as opposed to many position in the spec.

Spec:
https://github.com/microsoft/language-server-protocol/blob/dbaeumer/3.15/specification.md#textDocument_selectionRange

Reviewers: hokein

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

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

Added:
clang-tools-extra/trunk/clangd/test/selection-range.test
Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/clangd/test/initialize-params.test

Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=372753&r1=372752&r2=372753&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Tue Sep 24 06:38:33 2019
@@ -22,14 +22,18 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -127,6 +131,21 @@ llvm::Error validateEdits(const DraftSto
   llvm::to_string(InvalidFileCount - 1) + " others)");
 }
 
+// Converts a list of Ranges to a LinkedList of SelectionRange.
+SelectionRange render(const std::vector &Ranges) {
+  if (Ranges.empty())
+return {};
+  SelectionRange Result;
+  Result.range = Ranges[0];
+  auto *Next = &Result.parent;
+  for (const auto &R : llvm::make_range(Ranges.begin() + 1, Ranges.end())) {
+*Next = std::make_unique();
+Next->get()->range = R;
+Next = &Next->get()->parent;
+  }
+  return Result;
+}
+
 } // namespace
 
 // MessageHandler dispatches incoming LSP messages.
@@ -536,6 +555,7 @@ void ClangdLSPServer::onInitialize(const
 {"documentHighlightProvider", true},
 {"hoverProvider", true},
 {"renameProvider", std::move(RenameProvider)},
+{"selectionRangeProvider", true},
 {"documentSymbolProvider", true},
 {"workspaceSymbolProvider", true},
 {"referencesProvider", true},
@@ -1125,6 +1145,30 @@ void ClangdLSPServer::onSymbolInfo(const
  std::move(Reply));
 }
 
+void ClangdLSPServer::onSelectionRange(
+const SelectionRangeParams &Params,
+Callback> Reply) {
+  if (Params.positions.size() != 1) {
+elog("{0} positions provided to SelectionRange. Supports exactly one "
+ "position.",
+ Params.positions.size());
+return Reply(llvm::make_error(
+"SelectionRange supports exactly one position",
+ErrorCode::InvalidRequest));
+  }
+  Server->semanticRanges(
+  Params.textDocument.uri.file(), Params.positions[0],
+  [Reply = std::move(Reply)](
+  llvm::Expected> Ranges) mutable {
+if (!Ranges) {
+  return Reply(Ranges.takeError());
+}
+std::vector Result;
+Result.emplace_back(render(std::move(*Ranges)));
+return Reply(std::move(Result));
+  });
+}
+
 ClangdLSPServer::ClangdLSPServer(
 class Transport &Transp, const FileSystemProvider &FSProvider,
 const clangd::CodeCompleteOptions &CCOpts,
@@ -1167,6 +1211,7 @@ ClangdLSPServer::ClangdLSPServer(
   MsgHandler->bind("textDocument/symbolInfo", &ClangdLSPServer::onSymbolInfo);
   MsgHandler->bind("textDocument/typeHierarchy", 
&ClangdLSPServer::onTypeHierarchy);
   MsgHandler->bind("typeHierarchy/resolve", 
&ClangdLSPServer::onResolveTypeHierarchy);
+  MsgHandler->bind("textDocument/selectionRange", 
&ClangdLSPServer::onSelectionRange);
   // clang-format on
 }
 

Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=372753&r1=372752&r2=372753&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Tue Sep 24 06:38:33 2019
@

[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Kadir Cetinkaya via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/FindTarget.h:96
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ReferenceLoc R);
+

ilya-biryukov wrote:
> kadircet wrote:
> > Is this for testing purposes? maybe move it into `findtargettests.cpp` or 
> > make it a member helper like `Print(SourceManager&)` so that it can also 
> > print locations etc.?
> Yes, this is for output in the test. Moving to `findTargetTests.cpp`, we risk 
> ODR violations in the future.
> In any case, it's useful to have debug output and `operator<<` is common for 
> that purpose: it enables `llvm::toString` and, consecutively, 
> `llvm::formatv("{0}", R)`.
> 
> What are the downsides of having it?
> 
I wasn't happy about it because it actually needs a `SourceManager` to print 
completely, and I believe that's also necessary for debugging.

But feel free to ignore if you're OK with that.



Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:498
+std::string AnnotatedCode;
+unsigned NextOffset = 0;
+for (unsigned I = 0; I < Refs.size(); ++I) {

ilya-biryukov wrote:
> kadircet wrote:
> > this sounds more like `LastOffset` or `PrevOffset`
> That's the next character in `Code` we need to process, renamed to 
> `NextCodeChar` to specifically mention what string we're referring to.
well, this looked more like "beginning of the last token we processed" to me. 
but now I see your point of view as well. Feel free to use whichever you want.



Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:600
+   "5: targets = {a::b::S}\n"
+   "6: targets = {a::b::S::type}, qualifier = 'struct S::'\n"},
+  // Simple templates.

ilya-biryukov wrote:
> kadircet wrote:
> > Is it OK for this case to be different than `X::a` above?
> > 
> > also shouldn't this be `a::b::struct S` or `None` ?(my preference would be 
> > towards None)
> Qualifier captures exactly what's written in the source code, so it seems 
> correct: `S::` is what written down.
> `struct` comes from the printing function in clang and I don't think it's 
> worth changing. I believe this comes from the fact that qualifier is a type 
> in this case.
> 
> Why would the qualifier be `None`? Maybe the purpose of this field is unclear?
ah sorry, I missed the fact that this was written as `S::type` in the code, now 
it makes sense. just ignore this one.



Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:638
+   "0: targets = {y}\n"
+   "1: targets = {Y::foo}\n"},
+  };

ilya-biryukov wrote:
> kadircet wrote:
> > again qualifiers seems to be inconsistent with the rest of the cases.
> There are no qualifiers written in the source code in any of the two 
> references, therefore both qualifiers are empty.
> Again, please let me know if the purpose of `Qualifier` is unclear. Happy to 
> comment or change to make its purpose clear.
> 
yeah it totally makes sense now, the problem started with me missing the `S::` 
in the previous example :D, nvm..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826



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

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

[UTKARSH SAXENA via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372753: [clangd] Add semantic selection to ClangdLSPServer. 
(authored by usaxena95, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67720?vs=221534&id=221535#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67720

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/test/initialize-params.test
  clang-tools-extra/trunk/clangd/test/selection-range.test

Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h
@@ -107,6 +107,8 @@
   void onChangeConfiguration(const DidChangeConfigurationParams &);
   void onSymbolInfo(const TextDocumentPositionParams &,
 Callback>);
+  void onSelectionRange(const SelectionRangeParams &,
+Callback>);
 
   std::vector getFixes(StringRef File, const clangd::Diagnostic &D);
 
Index: clang-tools-extra/trunk/clangd/Protocol.cpp
===
--- clang-tools-extra/trunk/clangd/Protocol.cpp
+++ clang-tools-extra/trunk/clangd/Protocol.cpp
@@ -1073,5 +1073,18 @@
   };
 }
 
+bool fromJSON(const llvm::json::Value &Params, SelectionRangeParams &P) {
+  llvm::json::ObjectMapper O(Params);
+  return O && O.map("textDocument", P.textDocument) &&
+ O.map("positions", P.positions);
+}
+
+llvm::json::Value toJSON(const SelectionRange &Out) {
+  if (Out.parent) {
+return llvm::json::Object{{"range", Out.range},
+  {"parent", toJSON(*Out.parent)}};
+  }
+  return llvm::json::Object{{"range", Out.range}};
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/test/initialize-params.test
===
--- clang-tools-extra/trunk/clangd/test/initialize-params.test
+++ clang-tools-extra/trunk/clangd/test/initialize-params.test
@@ -33,6 +33,7 @@
 # CHECK-NEXT:  "hoverProvider": true,
 # CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
+# CHECK-NEXT:  "selectionRangeProvider": true,
 # CHECK-NEXT:  "signatureHelpProvider": {
 # CHECK-NEXT:"triggerCharacters": [
 # CHECK-NEXT:  "(",
Index: clang-tools-extra/trunk/clangd/test/selection-range.test
===
--- clang-tools-extra/trunk/clangd/test/selection-range.test
+++ clang-tools-extra/trunk/clangd/test/selection-range.test
@@ -0,0 +1,39 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void func() {\n}"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/selectionRange","params":{"textDocument":{"uri":"test:///main.cpp"},"positions":[{"line":1,"character":0}]}}
+#  CHECK:  "id": 1
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT:  "parent": {
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 1,
+# CHECK-NEXT:"line": 1
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 0,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT:  "character": 1,
+# CHECK-NEXT:  "line": 1
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT:  "character": 12,
+# CHECK-NEXT:  "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  ]
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/clangd/Protocol.h
===
--- clang-tools-extra/trunk/clangd/Protocol.h
+++ clang-tools-extra/trunk/clangd/Protocol.h
@@ -30,6 +30,7 @@
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -1222,6 +1223,28 @@
 };
 llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting);
 
+struct SelectionRangeParams {
+  /// The text document.
+  TextDocumentIdentifier te

r372760 - [clang-format] [PR36858] Add missing .hh and .cs extensions from python support utilities

[Paul Hoad via cfe-commits]

Author: paulhoad
Date: Tue Sep 24 07:00:06 2019
New Revision: 372760

URL: http://llvm.org/viewvc/llvm-project?rev=372760&view=rev
Log:
[clang-format] [PR36858] Add missing .hh and .cs extensions from python support 
utilities

Summary: https://bugs.llvm.org/show_bug.cgi?id=36858 identifies .hh as a 
missing C++ header extension file while making this change I realized there was 
no support for .cs files which were added recently

Reviewers: pseyfert, klimek, owenpan

Reviewed By: klimek

Subscribers: cfe-commits

Tags: #clang-tools-extra, #clang

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

Modified:
cfe/trunk/tools/clang-format/clang-format-diff.py
cfe/trunk/tools/clang-format/git-clang-format

Modified: cfe/trunk/tools/clang-format/clang-format-diff.py
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-format/clang-format-diff.py?rev=372760&r1=372759&r2=372760&view=diff
==
--- cfe/trunk/tools/clang-format/clang-format-diff.py (original)
+++ cfe/trunk/tools/clang-format/clang-format-diff.py Tue Sep 24 07:00:06 2019
@@ -43,8 +43,8 @@ def main():
   help='custom pattern selecting file paths to reformat '
   '(case sensitive, overrides -iregex)')
   parser.add_argument('-iregex', metavar='PATTERN', default=
-  r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hpp|m|mm|inc|js|ts|proto'
-  r'|protodevel|java)',
+  
r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hh|hpp|m|mm|inc|js|ts|proto'
+  r'|protodevel|java|cs)',
   help='custom pattern selecting file paths to reformat '
   '(case insensitive, overridden by -regex)')
   parser.add_argument('-sort-includes', action='store_true', default=False,

Modified: cfe/trunk/tools/clang-format/git-clang-format
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-format/git-clang-format?rev=372760&r1=372759&r2=372760&view=diff
==
--- cfe/trunk/tools/clang-format/git-clang-format (original)
+++ cfe/trunk/tools/clang-format/git-clang-format Tue Sep 24 07:00:06 2019
@@ -77,13 +77,14 @@ def main():
   'c', 'h',  # C
   'm',  # ObjC
   'mm',  # ObjC++
-  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hpp', 'hxx',  # C++
+  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hh', 'hpp', 'hxx',  # C++
   'cu',  # CUDA
   # Other languages that clang-format supports
   'proto', 'protodevel',  # Protocol Buffers
   'java',  # Java
   'js',  # JavaScript
   'ts',  # TypeScript
+  'cs',  # C Sharp
   ])
 
   p = argparse.ArgumentParser(


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

[PATCH] D67949: [clang-format] [PR36858] Add missing .hh and .cs extensions from python support utilities

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372760: [clang-format] [PR36858] Add missing .hh and .cs 
extensions from python support… (authored by paulhoad, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67949?vs=221474&id=221537#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67949

Files:
  cfe/trunk/tools/clang-format/clang-format-diff.py
  cfe/trunk/tools/clang-format/git-clang-format


Index: cfe/trunk/tools/clang-format/clang-format-diff.py
===
--- cfe/trunk/tools/clang-format/clang-format-diff.py
+++ cfe/trunk/tools/clang-format/clang-format-diff.py
@@ -43,8 +43,8 @@
   help='custom pattern selecting file paths to reformat '
   '(case sensitive, overrides -iregex)')
   parser.add_argument('-iregex', metavar='PATTERN', default=
-  r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hpp|m|mm|inc|js|ts|proto'
-  r'|protodevel|java)',
+  
r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hh|hpp|m|mm|inc|js|ts|proto'
+  r'|protodevel|java|cs)',
   help='custom pattern selecting file paths to reformat '
   '(case insensitive, overridden by -regex)')
   parser.add_argument('-sort-includes', action='store_true', default=False,
Index: cfe/trunk/tools/clang-format/git-clang-format
===
--- cfe/trunk/tools/clang-format/git-clang-format
+++ cfe/trunk/tools/clang-format/git-clang-format
@@ -77,13 +77,14 @@
   'c', 'h',  # C
   'm',  # ObjC
   'mm',  # ObjC++
-  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hpp', 'hxx',  # C++
+  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hh', 'hpp', 'hxx',  # C++
   'cu',  # CUDA
   # Other languages that clang-format supports
   'proto', 'protodevel',  # Protocol Buffers
   'java',  # Java
   'js',  # JavaScript
   'ts',  # TypeScript
+  'cs',  # C Sharp
   ])
 
   p = argparse.ArgumentParser(


Index: cfe/trunk/tools/clang-format/clang-format-diff.py
===
--- cfe/trunk/tools/clang-format/clang-format-diff.py
+++ cfe/trunk/tools/clang-format/clang-format-diff.py
@@ -43,8 +43,8 @@
   help='custom pattern selecting file paths to reformat '
   '(case sensitive, overrides -iregex)')
   parser.add_argument('-iregex', metavar='PATTERN', default=
-  r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hpp|m|mm|inc|js|ts|proto'
-  r'|protodevel|java)',
+  r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hh|hpp|m|mm|inc|js|ts|proto'
+  r'|protodevel|java|cs)',
   help='custom pattern selecting file paths to reformat '
   '(case insensitive, overridden by -regex)')
   parser.add_argument('-sort-includes', action='store_true', default=False,
Index: cfe/trunk/tools/clang-format/git-clang-format
===
--- cfe/trunk/tools/clang-format/git-clang-format
+++ cfe/trunk/tools/clang-format/git-clang-format
@@ -77,13 +77,14 @@
   'c', 'h',  # C
   'm',  # ObjC
   'mm',  # ObjC++
-  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hpp', 'hxx',  # C++
+  'cc', 'cp', 'cpp', 'c++', 'cxx', 'hh', 'hpp', 'hxx',  # C++
   'cu',  # CUDA
   # Other languages that clang-format supports
   'proto', 'protodevel',  # Protocol Buffers
   'java',  # Java
   'js',  # JavaScript
   'ts',  # TypeScript
+  'cs',  # C Sharp
   ])
 
   p = argparse.ArgumentParser(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67964: [clangd] Update vscode lsp dependencies to pickup the new changes in LSP v3.15.0.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: usaxena95.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.

This would enable the newly-added semantic selection feature in vscode.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67964

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/package.json


Index: clang-tools-extra/clangd/clients/clangd-vscode/package.json
===
--- clang-tools-extra/clangd/clients/clangd-vscode/package.json
+++ clang-tools-extra/clangd/clients/clangd-vscode/package.json
@@ -37,9 +37,9 @@
 },
 "dependencies": {
 "jsonc-parser": "^2.1.0",
-"vscode-languageclient": "^5.3.0-next.6",
-"vscode-languageserver": "^5.3.0-next.6",
-"vscode-languageserver-types": "^3.14.0"
+"vscode-languageclient": "^6.0.0-next.1",
+"vscode-languageserver": "^6.0.0-next.1",
+"vscode-languageserver-types": "^3.15.0-next.5"
 },
 "devDependencies": {
 "@types/mocha": "^2.2.32",


Index: clang-tools-extra/clangd/clients/clangd-vscode/package.json
===
--- clang-tools-extra/clangd/clients/clangd-vscode/package.json
+++ clang-tools-extra/clangd/clients/clangd-vscode/package.json
@@ -37,9 +37,9 @@
 },
 "dependencies": {
 "jsonc-parser": "^2.1.0",
-"vscode-languageclient": "^5.3.0-next.6",
-"vscode-languageserver": "^5.3.0-next.6",
-"vscode-languageserver-types": "^3.14.0"
+"vscode-languageclient": "^6.0.0-next.1",
+"vscode-languageserver": "^6.0.0-next.1",
+"vscode-languageserver-types": "^3.15.0-next.5"
 },
 "devDependencies": {
 "@types/mocha": "^2.2.32",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67877: [analyzer] Conditionnaly include clang Analysis examples with cmake.

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

Szelethus added a comment.

I have no objections on my end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67877



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

[PATCH] D67837: [CUDA][HIP] Fix assertion in Sema::markKnownEmitted with -fopenmp

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D67837#1679670 , @rjmccall wrote:

> Okay.  And it's okay to fall down to the code below when functions are used 
> in both ways this way?


This part of code is for delayed checking of hostness. If a host function calls 
device function, we do not want to diagnose it unless we are sure the host func 
is to be emitted.

whether a func is emitted is determined by IsKnownEmitted function. HIP and 
OpenMP each has its own copy.

In this case, HIP thinks a static host func is not sure to be emitted, unless 
it has an external linkage.

However, OpenMP thinks a static host func is always emitted.

If we do not consolidate IsKnownEmitted function of HIP and OpenMP, and use the 
current fix, for HIP, some additional diagnostics will be emitted compared to 
without -fopenmp in certain cases.

It seems I should figure out whether the static host func is really emitted for 
HIP then consolidate IsKnownEmitted func for HIP and OpenMP.


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

https://reviews.llvm.org/D67837



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

anton-afanasyev wrote:
> lebedev.ri wrote:
> > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", "Code 
> > Generation Time")` timer.
> Thanks, I'm to add it.
Hmm, I've figured out this isn't needed: such new timer mostly coincides with 
"Backend" timer (above). Legacy `Timer CodeGenerationTime(...)` is bad example 
of doing right timing.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675



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

[PATCH] D63978: Clang Interface Stubs merger plumbing for Driver

[Cyndy Ishida via Phabricator via cfe-commits]

cishida added inline comments.



Comment at: clang/include/clang/Driver/Options.td:635
+  Flags<[CC1Option]>, Group,
+  HelpText<"Generate Inteface Stub Files, emit merged text not binary.">;
 def iterface_stub_version_EQ : JoinedOrSeparate<["-"], 
"interface-stub-version=">, Flags<[CC1Option]>;

nit: Interface 



Comment at: clang/lib/Driver/ToolChains/InterfaceStubs.h:14
+#include "clang/Driver/ToolChain.h"
+#include 
+

is this used anywhere? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63978



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

[PATCH] D63978: Clang Interface Stubs merger plumbing for Driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:3372
+  if (Phase == phases::IfsMerge) {
+assert(Phase == PL.back() && "merging must be final compilation 
step.");
+MergerInputs.push_back(Current);

Does the interface merging have to be the last step?  I could see interface 
merging preceding linking just fine.



Comment at: clang/lib/Driver/Driver.cpp:3468
+  case phases::IfsMerge:
 llvm_unreachable("link action invalid here.");
   case phases::Preprocess: {

Please update the unreachable message



Comment at: clang/test/InterfaceStubs/driver-test.cpp:17
+// CHECK-DAG: _Z8weakFuncv
+
+int foo(int bar) {

Should we not ensure that no other symbols are exported?



Comment at: clang/test/InterfaceStubs/merge-conflict-test.cpp:3
+
+// -x c so that we dont have name-mangling.
+// RUN: not %clang -target x86_64-linux-gnu -x c -o libfoo.so 
-emit-interface-stubs \

Why not name the file `merge-conflict-test.c`?



Comment at: clang/test/InterfaceStubs/merge-conflict-test.cpp:4
+// -x c so that we dont have name-mangling.
+// RUN: not %clang -target x86_64-linux-gnu -x c -o libfoo.so 
-emit-interface-stubs \
+// RUN: %s %S/driver-test.cpp 2>&1 | FileCheck %s

Why not leave the `-target` off entirely?  That will allow you to drop the 
restriction of the `x86-registered-target`



Comment at: clang/test/InterfaceStubs/object-double.cpp:2
+// REQUIRES: x86-registered-target
+// RUN: not %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs 
%s %S/object.cpp 2>&1 | \
+// RUN: FileCheck %s

Similar



Comment at: clang/test/InterfaceStubs/object-float.cpp:2
+// REQUIRES: x86-registered-target
+// RUN: not %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs 
%s %S/object.cpp 2>&1 | \
+// RUN: FileCheck %s

And here



Comment at: clang/test/InterfaceStubs/object.cpp:5
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \
-// RUN: -interface-stub-version=experimental-ifs-v1 %s | \
+// RUN: %clang -c -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs 
%s | \
 // RUN: FileCheck -check-prefix=CHECK-SYMBOLS %s

Might as well as do it here as well



Comment at: clang/test/InterfaceStubs/object.ifs:1
+# RUN: %clang -emit-merged-ifs -target x86_64-unknown-linux-gnu \
+# RUN: -emit-interface-stubs  -o - %s | \

Can we drop the target and get this to be portable?



Comment at: clang/test/InterfaceStubs/template-namespace-function.cpp:2
 // REQUIRES: x86-registered-target
-// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \
-// RUN: -interface-stub-version=experimental-ifs-v1 %s | \
+// RUN: %clang -c -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs 
%s | \
 // RUN: FileCheck %s

Try to drop the x86 requirement throughout


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63978



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

[PATCH] D67713: [OpenCL] Add image query builtin functions

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D67713



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

[PATCH] D67714: [OpenCL] Add -Wconversion to fdeclare-opencl-builtins test

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl:102
 // expected-error@-2{{implicit declaration of function 'get_sub_group_size' is 
invalid in OpenCL}}
+// expected-error@-3{{implicit conversion changes signedness: 'int' to 'uint' 
(aka 'unsigned int')}}
 #endif

I am slightly confused about this error, isn't `get_sub_group_size` supposed to 
return uint?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67714



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

anton-afanasyev wrote:
> anton-afanasyev wrote:
> > lebedev.ri wrote:
> > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", "Code 
> > > Generation Time")` timer.
> > Thanks, I'm to add it.
> Hmm, I've figured out this isn't needed: such new timer mostly coincides with 
> "Backend" timer (above). Legacy `Timer CodeGenerationTime(...)` is bad 
> example of doing right timing.
"Mostly coincides" may not be the best way to approach fine-grained timings, i 
think? :)

I have noticed this because when i looked at the produced time flame graph,
there's large section in the end that is covered only by `"Backend"` timer,
but nothing else. Now, i'm not going to say whether or not that extra section
should or should not be within `"Backend"` timer, but it certainly should *also*
be within `"codegen"` timer. Or is there no codegen time spent there?
{F10062322}
{F10062316}


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675



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

[PATCH] D67714: [OpenCL] Add -Wconversion to fdeclare-opencl-builtins test

[Sven van Haastregt via Phabricator via cfe-commits]

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



Comment at: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl:102
 // expected-error@-2{{implicit declaration of function 'get_sub_group_size' is 
invalid in OpenCL}}
+// expected-error@-3{{implicit conversion changes signedness: 'int' to 'uint' 
(aka 'unsigned int')}}
 #endif

Anastasia wrote:
> I am slightly confused about this error, isn't `get_sub_group_size` supposed 
> to return uint?
I think this is a side-effect from the previous error.  It failed to resolve 
`get_sub_group_size` so it inserted a placeholder declaration with return type 
`int`.  Does that make sense?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67714



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

lebedev.ri wrote:
> anton-afanasyev wrote:
> > anton-afanasyev wrote:
> > > lebedev.ri wrote:
> > > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", "Code 
> > > > Generation Time")` timer.
> > > Thanks, I'm to add it.
> > Hmm, I've figured out this isn't needed: such new timer mostly coincides 
> > with "Backend" timer (above). Legacy `Timer CodeGenerationTime(...)` is bad 
> > example of doing right timing.
> "Mostly coincides" may not be the best way to approach fine-grained timings, 
> i think? :)
> 
> I have noticed this because when i looked at the produced time flame graph,
> there's large section in the end that is covered only by `"Backend"` timer,
> but nothing else. Now, i'm not going to say whether or not that extra section
> should or should not be within `"Backend"` timer, but it certainly should 
> *also*
> be within `"codegen"` timer. Or is there no codegen time spent there?
> {F10062322}
> {F10062316}
"Mostly coincides" here means "identical" I believe, the difference is 
auxiliary stuff.
Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer for 
`"codegen"` one.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675



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

[PATCH] D67216: [cfi] Add flag to always generate call frame information

[Keith Walker via Phabricator via cfe-commits]

keith.walker.arm added a comment.

I feel that using a -g option make more sense.

Maybe -gdwarf-frame, or -gdwarf-frame-always might be more user friendly as it 
relates more to the DWARF section created rather than the section content.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67216



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

[PATCH] D67947: [HIP] Support new kernel launching API

[Artem Belevich via Phabricator via cfe-commits]

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

LGTM.


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

https://reviews.llvm.org/D67947



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

Re: [PATCH] D67216: [cfi] Add flag to always generate call frame information

[Eric Christopher via cfe-commits]

That said, prior art in gcc in this area:

-fasynchronous-unwind-tables

Generate unwind table in DWARF format, if supported by target machine.
The table is exact at each instruction boundary, so it can be used for
stack unwinding from asynchronous events (such as debugger or garbage
collector).

-eric

On Tue, Sep 24, 2019 at 8:43 AM Keith Walker via Phabricator
 wrote:
>
> keith.walker.arm added a comment.
>
> I feel that using a -g option make more sense.
>
> Maybe -gdwarf-frame, or -gdwarf-frame-always might be more user friendly as 
> it relates more to the DWARF section created rather than the section content.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D67216/new/
>
> https://reviews.llvm.org/D67216
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67385: Pass -mcmodel to LTO plugin

[Kuan Hsu Chen via Phabricator via cfe-commits]

khchen added a comment.

@tejohnson for example:

  $ clang -flto a.c -c -o a.o
  $ llvm-ar q a.a a.o  // archive libraries  
  $ clang -flto a.a b.c -O2 -o main -mcmodel=small 

In above case user need to aware that passing 
`-Wl,-plugin-opt=-code-model=small` to plugin is necessary.
It seems to me that is inconvenience because I believe user doesn't know it.

I think this case is similar to if a user has a bitcode library compiled with 
-O2 or -Oz, and link a program with -O3 -flto, what is expected behavior for 
user?


Repository:
  rC Clang

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

https://reviews.llvm.org/D67385



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

anton-afanasyev wrote:
> lebedev.ri wrote:
> > anton-afanasyev wrote:
> > > anton-afanasyev wrote:
> > > > lebedev.ri wrote:
> > > > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", 
> > > > > "Code Generation Time")` timer.
> > > > Thanks, I'm to add it.
> > > Hmm, I've figured out this isn't needed: such new timer mostly coincides 
> > > with "Backend" timer (above). Legacy `Timer CodeGenerationTime(...)` is 
> > > bad example of doing right timing.
> > "Mostly coincides" may not be the best way to approach fine-grained 
> > timings, i think? :)
> > 
> > I have noticed this because when i looked at the produced time flame graph,
> > there's large section in the end that is covered only by `"Backend"` timer,
> > but nothing else. Now, i'm not going to say whether or not that extra 
> > section
> > should or should not be within `"Backend"` timer, but it certainly should 
> > *also*
> > be within `"codegen"` timer. Or is there no codegen time spent there?
> > {F10062322}
> > {F10062316}
> "Mostly coincides" here means "identical" I believe, the difference is 
> auxiliary stuff.
> Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer for 
> `"codegen"` one.
Then we are talking about different things using the same name.
There are two distinct codegen steps:
1. clang AST -> LLVM IR codegen
(after that, all the opt passes run)
2. LLVM IR -> final assembly. This happens after all the opt middle-end passes.

Those are *different* codegen's.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675



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

[PATCH] D67385: Pass -mcmodel to LTO plugin

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D67385#1680942 , @khchen wrote:

> @tejohnson for example:
>
>   $ clang -flto a.c -c -o a.o
>   $ llvm-ar q a.a a.o  // archive libraries  
>   $ clang -flto a.a b.c -O2 -o main -mcmodel=small 
>
>
> In above case user need to aware that passing 
> `-Wl,-plugin-opt=-code-model=small` to plugin is necessary.
>  It seems to me that is inconvenience because I believe user doesn't know it.


In general, -flto should be transparent to the build process. Think about what 
happens in your example if you remove -flto:

$ clang a.c -c -o a.o
$ llvm-ar q a.a a.o  // archive libraries  
$ clang a.a b.c -O2 -o main -mcmodel=small

a.o will be a native code built with the default mcmodel and -O0.

Generally, adding -flto shouldn't change this - I would argue that it would be 
unexpected to apply mcmodel=small as the user chose to compile a.c with the 
default mcmodel. We are going more and more toward a model where info is 
communicated via the IR from the compile step, as the mcmodel already is. Is 
there a compelling reason why a.c should not get the default mcmodel in this 
case (without having compiled it mcmodel=small in the first place)?

> I think this case is similar to if a user has a bitcode library compiled with 
> -O2 or -Oz, and link a program with -O3 -flto, what is expected behavior for 
> user?

There are a few things that are passed through the link command line to the 
plugin for legacy reasons, including -O2/-O3. However, note that in your above 
example the -O2 will not cause a.o to be O2 
 optimized: a.c is built at -O0 and 
this is encoded in the IR via function attributes (noinline and optnone), so 
the functions will not get O2 /O3 
 optimizations like inlining, etc.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67385



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 3 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

lebedev.ri wrote:
> anton-afanasyev wrote:
> > lebedev.ri wrote:
> > > anton-afanasyev wrote:
> > > > anton-afanasyev wrote:
> > > > > lebedev.ri wrote:
> > > > > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > > > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", 
> > > > > > "Code Generation Time")` timer.
> > > > > Thanks, I'm to add it.
> > > > Hmm, I've figured out this isn't needed: such new timer mostly 
> > > > coincides with "Backend" timer (above). Legacy `Timer 
> > > > CodeGenerationTime(...)` is bad example of doing right timing.
> > > "Mostly coincides" may not be the best way to approach fine-grained 
> > > timings, i think? :)
> > > 
> > > I have noticed this because when i looked at the produced time flame 
> > > graph,
> > > there's large section in the end that is covered only by `"Backend"` 
> > > timer,
> > > but nothing else. Now, i'm not going to say whether or not that extra 
> > > section
> > > should or should not be within `"Backend"` timer, but it certainly should 
> > > *also*
> > > be within `"codegen"` timer. Or is there no codegen time spent there?
> > > {F10062322}
> > > {F10062316}
> > "Mostly coincides" here means "identical" I believe, the difference is 
> > auxiliary stuff.
> > Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer for 
> > `"codegen"` one.
> Then we are talking about different things using the same name.
> There are two distinct codegen steps:
> 1. clang AST -> LLVM IR codegen
> (after that, all the opt passes run)
> 2. LLVM IR -> final assembly. This happens after all the opt middle-end 
> passes.
> 
> Those are *different* codegen's.
Yes, and step 1 is named as "CodeGen Function" whereas step 2 is named just 
"Backend".


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675



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

[PATCH] D67969: [libTooling] Add `run` combinator to Stencils.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a project: clang.

This revision adds `run`, a StencilPart that runs a user-defined function that
computes a result over `MatchFinder::MatchResult`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67969

Files:
  clang/include/clang/Tooling/Refactoring/Stencil.h
  clang/lib/Tooling/Refactoring/Stencil.cpp
  clang/unittests/Tooling/StencilTest.cpp

Index: clang/unittests/Tooling/StencilTest.cpp
===
--- clang/unittests/Tooling/StencilTest.cpp
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -29,6 +29,7 @@
 using stencil::cat;
 using stencil::dPrint;
 using stencil::ifBound;
+using stencil::run;
 using stencil::text;
 
 // In tests, we can't directly match on llvm::Expected since its accessors
@@ -300,6 +301,15 @@
   EXPECT_THAT_EXPECTED(Stencil.eval(StmtMatch->Result), HasValue("field"));
 }
 
+TEST_F(StencilTest, RunOp) {
+  StringRef Id = "id";
+  auto SimpleFn = [Id](const MatchResult &R) {
+return std::string(R.Nodes.getNodeAs(Id) != nullptr ? "Bound"
+  : "Unbound");
+  };
+  testExpr(Id, "3;", cat(run(SimpleFn)), "Bound");
+}
+
 TEST(StencilEqualityTest, Equality) {
   auto Lhs = cat("foo", dPrint("dprint_id"));
   auto Rhs = cat("foo", dPrint("dprint_id"));
@@ -324,4 +334,12 @@
   auto S2 = cat(node("node"));
   EXPECT_NE(S1, S2);
 }
+
+// `run` is opaque.
+TEST(StencilEqualityTest, InEqualityRun) {
+  auto F = [](const MatchResult &R) { return "foo"; };
+  auto S1 = cat(run(F));
+  auto S2 = cat(run(F));
+  EXPECT_NE(S1, S2);
+}
 } // namespace
Index: clang/lib/Tooling/Refactoring/Stencil.cpp
===
--- clang/lib/Tooling/Refactoring/Stencil.cpp
+++ clang/lib/Tooling/Refactoring/Stencil.cpp
@@ -26,6 +26,7 @@
 using ast_matchers::MatchFinder;
 using llvm::errc;
 using llvm::Error;
+using llvm::Expected;
 using llvm::StringError;
 
 // A down_cast function to safely down cast a StencilPartInterface to a subclass
@@ -102,6 +103,9 @@
   return A.Id == B.Id && A.TruePart == B.TruePart && A.FalsePart == B.FalsePart;
 }
 
+// Equality is not defined over MatchConsumers. They are, by definition, opaque.
+bool isEqualData(const MatchConsumer &A, const MatchConsumer &B) { return false; }
+
 // The `evalData()` overloads evaluate the given stencil data to a string, given
 // the match result, and append it to `Result`. We define an overload for each
 // type of stencil data.
@@ -159,6 +163,15 @@
   .eval(Match, Result);
 }
 
+Error evalData(const MatchConsumer &Fn, const MatchFinder::MatchResult &Match,
+   std::string *Result) {
+  Expected Value = Fn(Match);
+  if (!Value)
+return Value.takeError();
+  *Result += *Value;
+  return Error::success();
+}
+
 template 
 class StencilPartImpl : public StencilPartInterface {
   T Data;
@@ -233,3 +246,9 @@
   return StencilPart(std::make_shared>(
   Id, std::move(TruePart), std::move(FalsePart)));
 }
+
+StencilPart stencil::run(MatchConsumer Fn) {
+  return StencilPart(
+  std::make_shared>>(
+  std::move(Fn)));
+}
Index: clang/include/clang/Tooling/Refactoring/Stencil.h
===
--- clang/include/clang/Tooling/Refactoring/Stencil.h
+++ clang/include/clang/Tooling/Refactoring/Stencil.h
@@ -23,6 +23,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/MatchConsumer.h"
 #include "clang/Tooling/Refactoring/RangeSelector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -175,6 +176,10 @@
   return ifBound(Id, text(TrueText), text(FalseText));
 }
 
+/// Wraps a MatchConsumer in a StencilPart, so that it can be used in a Stencil.
+/// This supports user-defined extensions to the Stencil language.
+StencilPart run(MatchConsumer C);
+
 /// For debug use only; semantics are not guaranteed.
 ///
 /// \returns the string resulting from calling the node's print() method.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67969: [libTooling] Add `run` combinator to Stencils.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 221554.
ymandel added a comment.

clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67969

Files:
  clang/include/clang/Tooling/Refactoring/Stencil.h
  clang/lib/Tooling/Refactoring/Stencil.cpp
  clang/unittests/Tooling/StencilTest.cpp

Index: clang/unittests/Tooling/StencilTest.cpp
===
--- clang/unittests/Tooling/StencilTest.cpp
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -29,6 +29,7 @@
 using stencil::cat;
 using stencil::dPrint;
 using stencil::ifBound;
+using stencil::run;
 using stencil::text;
 
 // In tests, we can't directly match on llvm::Expected since its accessors
@@ -300,6 +301,15 @@
   EXPECT_THAT_EXPECTED(Stencil.eval(StmtMatch->Result), HasValue("field"));
 }
 
+TEST_F(StencilTest, RunOp) {
+  StringRef Id = "id";
+  auto SimpleFn = [Id](const MatchResult &R) {
+return std::string(R.Nodes.getNodeAs(Id) != nullptr ? "Bound"
+  : "Unbound");
+  };
+  testExpr(Id, "3;", cat(run(SimpleFn)), "Bound");
+}
+
 TEST(StencilEqualityTest, Equality) {
   auto Lhs = cat("foo", dPrint("dprint_id"));
   auto Rhs = cat("foo", dPrint("dprint_id"));
@@ -324,4 +334,12 @@
   auto S2 = cat(node("node"));
   EXPECT_NE(S1, S2);
 }
+
+// `run` is opaque.
+TEST(StencilEqualityTest, InEqualityRun) {
+  auto F = [](const MatchResult &R) { return "foo"; };
+  auto S1 = cat(run(F));
+  auto S2 = cat(run(F));
+  EXPECT_NE(S1, S2);
+}
 } // namespace
Index: clang/lib/Tooling/Refactoring/Stencil.cpp
===
--- clang/lib/Tooling/Refactoring/Stencil.cpp
+++ clang/lib/Tooling/Refactoring/Stencil.cpp
@@ -26,6 +26,7 @@
 using ast_matchers::MatchFinder;
 using llvm::errc;
 using llvm::Error;
+using llvm::Expected;
 using llvm::StringError;
 
 // A down_cast function to safely down cast a StencilPartInterface to a subclass
@@ -102,6 +103,12 @@
   return A.Id == B.Id && A.TruePart == B.TruePart && A.FalsePart == B.FalsePart;
 }
 
+// Equality is not defined over MatchConsumers, which are opaque.
+bool isEqualData(const MatchConsumer &A,
+ const MatchConsumer &B) {
+  return false;
+}
+
 // The `evalData()` overloads evaluate the given stencil data to a string, given
 // the match result, and append it to `Result`. We define an overload for each
 // type of stencil data.
@@ -159,6 +166,15 @@
   .eval(Match, Result);
 }
 
+Error evalData(const MatchConsumer &Fn,
+   const MatchFinder::MatchResult &Match, std::string *Result) {
+  Expected Value = Fn(Match);
+  if (!Value)
+return Value.takeError();
+  *Result += *Value;
+  return Error::success();
+}
+
 template 
 class StencilPartImpl : public StencilPartInterface {
   T Data;
@@ -233,3 +249,9 @@
   return StencilPart(std::make_shared>(
   Id, std::move(TruePart), std::move(FalsePart)));
 }
+
+StencilPart stencil::run(MatchConsumer Fn) {
+  return StencilPart(
+  std::make_shared>>(
+  std::move(Fn)));
+}
Index: clang/include/clang/Tooling/Refactoring/Stencil.h
===
--- clang/include/clang/Tooling/Refactoring/Stencil.h
+++ clang/include/clang/Tooling/Refactoring/Stencil.h
@@ -23,6 +23,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/MatchConsumer.h"
 #include "clang/Tooling/Refactoring/RangeSelector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -175,6 +176,10 @@
   return ifBound(Id, text(TrueText), text(FalseText));
 }
 
+/// Wraps a MatchConsumer in a StencilPart, so that it can be used in a Stencil.
+/// This supports user-defined extensions to the Stencil language.
+StencilPart run(MatchConsumer C);
+
 /// For debug use only; semantics are not guaranteed.
 ///
 /// \returns the string resulting from calling the node's print() method.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66795: [Mips] Use appropriate private label prefix based on Mips ABI

[Simon Atanasyan via Phabricator via cfe-commits]

atanasyan accepted this revision.
atanasyan added a comment.

LGTM. But before commit get more approvals. For example, from echristo, code 
owners of other targets, etc.

I would keep the as-is. In that case a target of such huge modifications looks 
a bit more clear.


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

https://reviews.llvm.org/D66795



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

[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 221556.
ilya-biryukov marked 8 inline comments as done.
ilya-biryukov added a comment.

- Remove ExprMemberBase
- Add an overload for Decl*


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/FindTarget.h
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -9,6 +9,9 @@
 
 #include "Selection.h"
 #include "TestTU.h"
+#include "clang/AST/Decl.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/Annotations.h"
 #include "gmock/gmock.h"
@@ -464,6 +467,223 @@
   EXPECT_DECLS("ObjCObjectTypeLoc");
 }
 
+class FindExplicitReferencesTest : public ::testing::Test {
+protected:
+  struct AllRefs {
+std::string AnnotatedCode;
+std::string DumpedReferences;
+  };
+
+  /// Parses \p Code, finds function '::foo' and annotates its body with results
+  /// of findExplicitReferecnces.
+  /// See actual tests for examples of annotation format.
+  AllRefs annotateReferencesInFoo(llvm::StringRef Code) {
+TestTU TU;
+TU.Code = Code;
+
+auto AST = TU.build();
+auto &Func = llvm::cast(findDecl(AST, "foo"));
+
+std::vector Refs;
+findExplicitReferences(Func.getBody(), [&Refs](ReferenceLoc R) {
+  Refs.push_back(std::move(R));
+});
+
+auto &SM = AST.getSourceManager();
+llvm::sort(Refs, [&](const ReferenceLoc &L, const ReferenceLoc &R) {
+  return SM.isBeforeInTranslationUnit(L.NameLoc, R.NameLoc);
+});
+
+std::string AnnotatedCode;
+unsigned NextCodeChar = 0;
+for (unsigned I = 0; I < Refs.size(); ++I) {
+  auto &R = Refs[I];
+
+  SourceLocation Pos = R.NameLoc;
+  assert(Pos.isValid());
+  if (Pos.isMacroID()) // FIXME: figure out how to show macro locations.
+Pos = SM.getExpansionLoc(Pos);
+  assert(Pos.isFileID());
+
+  FileID File;
+  unsigned Offset;
+  std::tie(File, Offset) = SM.getDecomposedLoc(Pos);
+  if (File == SM.getMainFileID()) {
+// Print the reference in a source code.
+assert(NextCodeChar <= Offset);
+AnnotatedCode += Code.substr(NextCodeChar, Offset - NextCodeChar);
+AnnotatedCode += "$" + std::to_string(I) + "^";
+
+NextCodeChar = Offset;
+  }
+}
+AnnotatedCode += Code.substr(NextCodeChar);
+
+std::string DumpedReferences;
+for (unsigned I = 0; I < Refs.size(); ++I)
+  DumpedReferences += llvm::formatv("{0}: {1}\n", I, Refs[I]);
+
+return AllRefs{std::move(AnnotatedCode), std::move(DumpedReferences)};
+  }
+};
+
+TEST_F(FindExplicitReferencesTest, All) {
+  std::pair Cases[] =
+  {
+  // Simple expressions.
+  {R"cpp(
+int global;
+int func();
+void foo(int param) {
+  $0^global = $1^param + $2^func();
+}
+)cpp",
+   "0: targets = {global}\n"
+   "1: targets = {param}\n"
+   "2: targets = {func}\n"},
+  {R"cpp(
+struct X { int a; };
+void foo(X x) {
+  $0^x.$1^a = 10;
+}
+)cpp",
+   "0: targets = {x}\n"
+   "1: targets = {X::a}\n"},
+  // Namespaces and aliases.
+  {R"cpp(
+  namespace ns {}
+  namespace alias = ns;
+  void foo() {
+using namespace $0^ns;
+using namespace $1^alias;
+  }
+)cpp",
+   "0: targets = {ns}\n"
+   "1: targets = {alias}\n"},
+  // Using declarations.
+  {R"cpp(
+  namespace ns { int global; }
+  void foo() {
+using $0^ns::$1^global;
+  }
+)cpp",
+   "0: targets = {ns}\n"
+   "1: targets = {ns::global}, qualifier = 'ns::'\n"},
+  // Simple types.
+  {R"cpp(
+ struct Struct { int a; };
+ using Typedef = int;
+ void foo() {
+   $0^Struct x;
+   $1^Typedef y;
+   static_cast<$2^Struct*>(0);
+ }
+   )cpp",
+   "0: targets = {Struct}\n"
+   "1: targets = {Typedef}\n"
+   "2: targets = {Struct}\n"},
+  // Name qualifiers.
+  {R"cpp(
+ namespace a { namespace b { struct S { typedef int type; }; } }
+ void foo() {
+   $0^a::$1^b::$2^S x;
+   using namespace $3^a::$4^b;
+   $5^S::$6^type y;
+ }
+)cpp",
+   "0: targets = {a}\n"
+   "1: targets = {a::b}, qualifier = 'a::'\n"
+   "2: targets = {a::b::S}, qualifier = 'a::b::'\n"
+   "3: targets = {a}\

[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I believe all comments were addressed. PTAL and let me know if I missed 
something.




Comment at: clang-tools-extra/clangd/FindTarget.h:93
+  /// in 'obj.foo'.
+  const Expr* MemberExprBase = nullptr;
+};

kadircet wrote:
> do we have any real use-case for this field? apart from "skipping instance 
> members in define inline"? because that one can also be achieved easily 
> through decl (there is a `isCXXInstanceMember`).
Thanks, this is definitely a valid concern. I was overthinking the problem, 
removed it.



Comment at: clang-tools-extra/clangd/FindTarget.h:115
+/// FIXME: extend to report location information about declaration names too.
+void findExplicitReferences(Stmt *S,
+llvm::function_ref Out);

kadircet wrote:
> It might be better to accept a `DynTypedNode` in here as well, so that this 
> can be used with other node types as well, or provide overrides for them(for 
> example `Decl`).
I would avoid doing `DynTypedNode` - many of the cases don't quite make sense 
here, e.g. `TypeLoc` is arguably ok as it has location information, but `Type*` 
or `QualType` are not ok as they lack location information.

`Decl` definitely sounds useful, though. Would allow to visit all top-level 
decls of a file, which is another use-case we probably want. Added that.



Comment at: clang-tools-extra/clangd/FindTarget.h:96
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ReferenceLoc R);
+

kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > Is this for testing purposes? maybe move it into `findtargettests.cpp` or 
> > > make it a member helper like `Print(SourceManager&)` so that it can also 
> > > print locations etc.?
> > Yes, this is for output in the test. Moving to `findTargetTests.cpp`, we 
> > risk ODR violations in the future.
> > In any case, it's useful to have debug output and `operator<<` is common 
> > for that purpose: it enables `llvm::toString` and, consecutively, 
> > `llvm::formatv("{0}", R)`.
> > 
> > What are the downsides of having it?
> > 
> I wasn't happy about it because it actually needs a `SourceManager` to print 
> completely, and I believe that's also necessary for debugging.
> 
> But feel free to ignore if you're OK with that.
Having some `llvm::toString` is great, because it's nicely integrated to all of 
the infrastructure and very easy to use.
I'll stick with it for now, despite the limitations arising from the absence of 
source manager.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

anton-afanasyev wrote:
> lebedev.ri wrote:
> > anton-afanasyev wrote:
> > > lebedev.ri wrote:
> > > > anton-afanasyev wrote:
> > > > > anton-afanasyev wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > This isn't covered by any timer; if you look in `BackendUtil.cpp`,
> > > > > > > `EmitAssemblyHelper` actually has `CodeGenerationTime("codegen", 
> > > > > > > "Code Generation Time")` timer.
> > > > > > Thanks, I'm to add it.
> > > > > Hmm, I've figured out this isn't needed: such new timer mostly 
> > > > > coincides with "Backend" timer (above). Legacy `Timer 
> > > > > CodeGenerationTime(...)` is bad example of doing right timing.
> > > > "Mostly coincides" may not be the best way to approach fine-grained 
> > > > timings, i think? :)
> > > > 
> > > > I have noticed this because when i looked at the produced time flame 
> > > > graph,
> > > > there's large section in the end that is covered only by `"Backend"` 
> > > > timer,
> > > > but nothing else. Now, i'm not going to say whether or not that extra 
> > > > section
> > > > should or should not be within `"Backend"` timer, but it certainly 
> > > > should *also*
> > > > be within `"codegen"` timer. Or is there no codegen time spent there?
> > > > {F10062322}
> > > > {F10062316}
> > > "Mostly coincides" here means "identical" I believe, the difference is 
> > > auxiliary stuff.
> > > Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer 
> > > for `"codegen"` one.
> > Then we are talking about different things using the same name.
> > There are two distinct codegen steps:
> > 1. clang AST -> LLVM IR codegen
> > (after that, all the opt passes run)
> > 2. LLVM IR -> final assembly. This happens after all the opt middle-end 
> > passes.
> > 
> > Those are *different* codegen's.
> Yes, and step 1 is named as "CodeGen Function" whereas step 2 is named just 
> "Backend".
Aha. So this is what i //expected// to see, apparently: 
{F10063128} {F10063119}
```
diff --git a/clang/lib/CodeGen/BackendUtil.cpp 
b/clang/lib/CodeGen/BackendUtil.cpp
index 71ae8fd4fb0..d89d12612f8 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -910,6 +910,7 @@ void EmitAssemblyHelper::EmitAssembly(BackendAction Action,
 
   {
 PrettyStackTraceString CrashInfo("Code generation");
+llvm::TimeTraceScope TimeScope("BackendCodegen", StringRef(""));
 CodeGenPasses.run(*TheModule);
   }
```

But that actually brings me to another question - what about 
`PrettyStackTraceString CrashInfo("Per-function optimization");`?
Should that be wrapped into some section, too?
I'm less certain here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675



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

Re: [PATCH] D67216: [cfi] Add flag to always generate call frame information

[John Reagan via cfe-commits]

Does the current code generate all the prologue cfi directives? 
Epilogue too?  The last time I checked, it did not (especially for
epilogues).

Fully asynch prologue/epilogue is one of the things that we need for
OpenVMS and will be doing that work.  We're also looking at using the
compact format instead of the CFI form (at least for routine bodies).

clang's driver already recognizes "-fasynchronous-unwind-tables" today
and maps that to "-munwind-tables" and probably should be switched to
setting your new "always-need-cfi" or simply change your name.    Either
way, I think there is an interaction between them.

From the code in Clang.cpp

  // This is a coarse approximation of what llvm-gcc actually does, both
  // -fasynchronous-unwind-tables and -fnon-call-exceptions interact in more
  // complicated ways.

On 9/24/19 11:54 AM, Eric Christopher wrote:
> That said, prior art in gcc in this area:
>
> -fasynchronous-unwind-tables
>
> Generate unwind table in DWARF format, if supported by target machine.
> The table is exact at each instruction boundary, so it can be used for
> stack unwinding from asynchronous events (such as debugger or garbage
> collector).
>
> -eric
>
> On Tue, Sep 24, 2019 at 8:43 AM Keith Walker via Phabricator
>  wrote:
>> keith.walker.arm added a comment.
>>
>> I feel that using a -g option make more sense.
>>
>> Maybe -gdwarf-frame, or -gdwarf-frame-always might be more user friendly as 
>> it relates more to the DWARF section created rather than the section content.
>>
>>
>> Repository:
>>   rG LLVM Github Monorepo
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D67216/new/
>>
>> https://reviews.llvm.org/D67216
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67216: [cfi] Add flag to always generate call frame information

[John Reagan via Phabricator via cfe-commits]

JohnReagan added a comment.

Does the current code generate all the prologue cfi directives? 
Epilogue too?  The last time I checked, it did not (especially for
epilogues).

Fully asynch prologue/epilogue is one of the things that we need for
OpenVMS and will be doing that work.  We're also looking at using the
compact format instead of the CFI form (at least for routine bodies).

clang's driver already recognizes "-fasynchronous-unwind-tables" today
and maps that to "-munwind-tables" and probably should be switched to
setting your new "always-need-cfi" or simply change your name.Either
way, I think there is an interaction between them.

From the code in Clang.cpp

  // This is a coarse approximation of what llvm-gcc actually does, both
  // -fasynchronous-unwind-tables and -fnon-call-exceptions interact in more
  // complicated ways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67216



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

[PATCH] D67971: [WIP] Add warning when zero-initializing a union if its first member is not is largest

[Leonard Chan via Phabricator via cfe-commits]

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

This may result in the entire union not being initialized. For example:

  union U {
uint32_t x;
uint64_t y;
  };
  
  void func() {
U u = {};
...
  }

Some might assume that `U u = {}` zero-initializes the whole union whereas only 
the bits that compose `U.x` are zero-inited.

Oddly enough (mostly from what I've seen personally), this seems to only matter 
in the case we use `-ftrivial-auto-var-init=pattern` where llvm `undef`s are 
replaced with non-zero values. In any normal case, it seems that these `undef`s 
just get lowered to zeros.

See: http://lists.llvm.org/pipermail/cfe-dev/2019-September/063383.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67971

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2041,8 +2041,16 @@
 CheckEmptyInitializable(
 InitializedEntity::InitializeMember(*Field, &Entity),
 IList->getEndLoc());
-if (StructuredList)
+if (StructuredList) {
   StructuredList->setInitializedFieldInUnion(*Field);
+
+  ASTContext &Ctx = SemaRef.Context;
+  unsigned UnionSize = Ctx.getTypeSize(Ctx.getRecordType(RD));
+  unsigned FieldSize = Ctx.getTypeSize(Field->getType());
+  if (UnionSize > FieldSize)
+SemaRef.Diag(IList->getLBraceLoc(), diag::warn_union_zero_init)
+  << RD->getName() << Field->getName();
+}
 break;
   }
 }
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9898,4 +9898,7 @@
   "__builtin_bit_cast %select{source|destination}0 type must be trivially 
copyable">;
 def err_bit_cast_type_size_mismatch : Error<
   "__builtin_bit_cast source size does not equal destination size (%0 vs %1)">;
+
+def warn_union_zero_init : Warning<
+  "zero-initializing union '%0' whose first non-static named data member '%1' 
is not the largest in the union will leave uninitialized bits in the rest of 
the union">;
 } // end of sema component.


Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -2041,8 +2041,16 @@
 CheckEmptyInitializable(
 InitializedEntity::InitializeMember(*Field, &Entity),
 IList->getEndLoc());
-if (StructuredList)
+if (StructuredList) {
   StructuredList->setInitializedFieldInUnion(*Field);
+
+  ASTContext &Ctx = SemaRef.Context;
+  unsigned UnionSize = Ctx.getTypeSize(Ctx.getRecordType(RD));
+  unsigned FieldSize = Ctx.getTypeSize(Field->getType());
+  if (UnionSize > FieldSize)
+SemaRef.Diag(IList->getLBraceLoc(), diag::warn_union_zero_init)
+  << RD->getName() << Field->getName();
+}
 break;
   }
 }
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9898,4 +9898,7 @@
   "__builtin_bit_cast %select{source|destination}0 type must be trivially copyable">;
 def err_bit_cast_type_size_mismatch : Error<
   "__builtin_bit_cast source size does not equal destination size (%0 vs %1)">;
+
+def warn_union_zero_init : Warning<
+  "zero-initializing union '%0' whose first non-static named data member '%1' is not the largest in the union will leave uninitialized bits in the rest of the union">;
 } // end of sema component.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67385: Pass -mcmodel to LTO plugin

[Kuan Hsu Chen via Phabricator via cfe-commits]

khchen added a comment.

In D67385#1680959 , @tejohnson wrote:

> In D67385#1680942 , @khchen wrote:
>
> > @tejohnson for example:
> >
> >   $ clang -flto a.c -c -o a.o
> >   $ llvm-ar q a.a a.o  // archive libraries  
> >   $ clang -flto a.a b.c -O2 -o main -mcmodel=small 
> >
> >
> > In above case user need to aware that passing 
> > `-Wl,-plugin-opt=-code-model=small` to plugin is necessary.
> >  It seems to me that is inconvenience because I believe user doesn't know 
> > it.
>
>
> In general, -flto should be transparent to the build process. Think about 
> what happens in your example if you remove -flto:
>
> $ clang a.c -c -o a.o
>  $ llvm-ar q a.a a.o  // archive libraries  
>  $ clang a.a b.c -O2 -o main -mcmodel=small
>
> a.o will be a native code built with the default mcmodel and -O0.
>
> Generally, adding -flto shouldn't change this - I would argue that it would 
> be unexpected to apply mcmodel=small as the user chose to compile a.c with 
> the default mcmodel. We are going more and more toward a model where info is 
> communicated via the IR from the compile step, as the mcmodel already is. Is 
> there a compelling reason why a.c should not get the default mcmodel in this 
> case (without having compiled it mcmodel=small in the first place)?


Because this library (a.c) is provided by another user/developer.
But you are right, if -flto should be transparent to the build process, user 
should be aware that.

>> I think this case is similar to if a user has a bitcode library compiled 
>> with -O2 or -Oz, and link a program with -O3 -flto, what is expected 
>> behavior for user?
> 
> There are a few things that are passed through the link command line to the 
> plugin for legacy reasons, including -O2/-O3. However, note that in your 
> above example the -O2 will not cause a.o to be O2 
>  optimized: a.c is built at -O0 
> and this is encoded in the IR via function attributes (noinline and optnone), 
> so the functions will not get O2 
> /O3 
>  optimizations like inlining, etc.

Got it, that's helpful.
Thanks for your time and effort.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67385



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

[PATCH] D67973: [libTooling][NFC] Switch StencilTest.cpp to use EXPECT_THAT_EXPECTED

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: ilya-biryukov.
Herald added a project: clang.

Currently, some tests use homegrown matchers to handle `llvm::Expected`
values. This revision standardizes on EXPECT_THAT_EXPECTED and `HasValue`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67973

Files:
  clang/unittests/Tooling/StencilTest.cpp


Index: clang/unittests/Tooling/StencilTest.cpp
===
--- clang/unittests/Tooling/StencilTest.cpp
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -31,23 +31,6 @@
 using stencil::ifBound;
 using stencil::text;
 
-// In tests, we can't directly match on llvm::Expected since its accessors
-// mutate the object. So, we collapse it to an Optional.
-static llvm::Optional toOptional(llvm::Expected V) {
-  if (V)
-return *V;
-  ADD_FAILURE() << "Losing error in conversion to IsSomething: "
-<< llvm::toString(V.takeError());
-  return llvm::None;
-}
-
-// A very simple matcher for llvm::Optional values.
-MATCHER_P(IsSomething, ValueMatcher, "") {
-  if (!arg)
-return false;
-  return ::testing::ExplainMatchResult(ValueMatcher, *arg, result_listener);
-}
-
 // Create a valid translation-unit from a statement.
 static std::string wrapSnippet(StringRef StatementCode) {
   return ("struct S { int field; }; auto stencil_test_snippet = []{" +
@@ -151,8 +134,8 @@
   // Invert the if-then-else.
   auto Stencil = cat("if (!", node(Condition), ") ", statement(Else), " else ",
  statement(Then));
-  EXPECT_THAT(toOptional(Stencil.eval(StmtMatch->Result)),
-  IsSomething(Eq("if (!true) return 0; else return 1;")));
+  EXPECT_THAT_EXPECTED(Stencil.eval(StmtMatch->Result),
+   HasValue("if (!true) return 0; else return 1;"));
 }
 
 TEST_F(StencilTest, SingleStatementCallOperator) {
@@ -170,8 +153,8 @@
   // Invert the if-then-else.
   Stencil S = cat("if (!", node(Condition), ") ", statement(Else), " else ",
   statement(Then));
-  EXPECT_THAT(toOptional(S(StmtMatch->Result)),
-  IsSomething(Eq("if (!true) return 0; else return 1;")));
+  EXPECT_THAT_EXPECTED(S(StmtMatch->Result),
+   HasValue("if (!true) return 0; else return 1;"));
 }
 
 TEST_F(StencilTest, UnboundNode) {


Index: clang/unittests/Tooling/StencilTest.cpp
===
--- clang/unittests/Tooling/StencilTest.cpp
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -31,23 +31,6 @@
 using stencil::ifBound;
 using stencil::text;
 
-// In tests, we can't directly match on llvm::Expected since its accessors
-// mutate the object. So, we collapse it to an Optional.
-static llvm::Optional toOptional(llvm::Expected V) {
-  if (V)
-return *V;
-  ADD_FAILURE() << "Losing error in conversion to IsSomething: "
-<< llvm::toString(V.takeError());
-  return llvm::None;
-}
-
-// A very simple matcher for llvm::Optional values.
-MATCHER_P(IsSomething, ValueMatcher, "") {
-  if (!arg)
-return false;
-  return ::testing::ExplainMatchResult(ValueMatcher, *arg, result_listener);
-}
-
 // Create a valid translation-unit from a statement.
 static std::string wrapSnippet(StringRef StatementCode) {
   return ("struct S { int field; }; auto stencil_test_snippet = []{" +
@@ -151,8 +134,8 @@
   // Invert the if-then-else.
   auto Stencil = cat("if (!", node(Condition), ") ", statement(Else), " else ",
  statement(Then));
-  EXPECT_THAT(toOptional(Stencil.eval(StmtMatch->Result)),
-  IsSomething(Eq("if (!true) return 0; else return 1;")));
+  EXPECT_THAT_EXPECTED(Stencil.eval(StmtMatch->Result),
+   HasValue("if (!true) return 0; else return 1;"));
 }
 
 TEST_F(StencilTest, SingleStatementCallOperator) {
@@ -170,8 +153,8 @@
   // Invert the if-then-else.
   Stencil S = cat("if (!", node(Condition), ") ", statement(Else), " else ",
   statement(Then));
-  EXPECT_THAT(toOptional(S(StmtMatch->Result)),
-  IsSomething(Eq("if (!true) return 0; else return 1;")));
+  EXPECT_THAT_EXPECTED(S(StmtMatch->Result),
+   HasValue("if (!true) return 0; else return 1;"));
 }
 
 TEST_F(StencilTest, UnboundNode) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67837: [CUDA][HIP] Fix assertion in Sema::markKnownEmitted with -fopenmp

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

If that's tractable, then that does seem like the best solution.  You can 
commit this if you need a shorter-term fix.


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

https://reviews.llvm.org/D67837



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

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Ed Maste via Phabricator via cfe-commits]

emaste added a comment.

In D60748#1499756 , @dim wrote:

> Please also exclude FreeBSD from these changes, since we care a lot about 
> backwards compatibility, and specifically about alignment requirements.  (We 
> have run into many issues in our ports collection where upstream assumes 
> everything is 16-byte aligned on i386, which is *NOT* ABI compliant.)


@dim that said I think we'd expect to be able to mix gcc- and clang-built 
objects, and it seems this is addressing somewhat of a corner case?


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

https://reviews.llvm.org/D60748



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added inline comments.



Comment at: cfe/trunk/lib/CodeGen/BackendUtil.cpp:1426-1431
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
 AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
 AsmHelper.EmitAssembly(Action, std::move(OS));

lebedev.ri wrote:
> anton-afanasyev wrote:
> > lebedev.ri wrote:
> > > anton-afanasyev wrote:
> > > > lebedev.ri wrote:
> > > > > anton-afanasyev wrote:
> > > > > > anton-afanasyev wrote:
> > > > > > > lebedev.ri wrote:
> > > > > > > > This isn't covered by any timer; if you look in 
> > > > > > > > `BackendUtil.cpp`,
> > > > > > > > `EmitAssemblyHelper` actually has 
> > > > > > > > `CodeGenerationTime("codegen", "Code Generation Time")` timer.
> > > > > > > Thanks, I'm to add it.
> > > > > > Hmm, I've figured out this isn't needed: such new timer mostly 
> > > > > > coincides with "Backend" timer (above). Legacy `Timer 
> > > > > > CodeGenerationTime(...)` is bad example of doing right timing.
> > > > > "Mostly coincides" may not be the best way to approach fine-grained 
> > > > > timings, i think? :)
> > > > > 
> > > > > I have noticed this because when i looked at the produced time flame 
> > > > > graph,
> > > > > there's large section in the end that is covered only by `"Backend"` 
> > > > > timer,
> > > > > but nothing else. Now, i'm not going to say whether or not that extra 
> > > > > section
> > > > > should or should not be within `"Backend"` timer, but it certainly 
> > > > > should *also*
> > > > > be within `"codegen"` timer. Or is there no codegen time spent there?
> > > > > {F10062322}
> > > > > {F10062316}
> > > > "Mostly coincides" here means "identical" I believe, the difference is 
> > > > auxiliary stuff.
> > > > Please look at `clang::EmitBackendOutput()`, `"Backend"` timer is outer 
> > > > for `"codegen"` one.
> > > Then we are talking about different things using the same name.
> > > There are two distinct codegen steps:
> > > 1. clang AST -> LLVM IR codegen
> > > (after that, all the opt passes run)
> > > 2. LLVM IR -> final assembly. This happens after all the opt middle-end 
> > > passes.
> > > 
> > > Those are *different* codegen's.
> > Yes, and step 1 is named as "CodeGen Function" whereas step 2 is named just 
> > "Backend".
> Aha. So this is what i //expected// to see, apparently: 
> {F10063128} {F10063119}
> ```
> diff --git a/clang/lib/CodeGen/BackendUtil.cpp 
> b/clang/lib/CodeGen/BackendUtil.cpp
> index 71ae8fd4fb0..d89d12612f8 100644
> --- a/clang/lib/CodeGen/BackendUtil.cpp
> +++ b/clang/lib/CodeGen/BackendUtil.cpp
> @@ -910,6 +910,7 @@ void EmitAssemblyHelper::EmitAssembly(BackendAction 
> Action,
>  
>{
>  PrettyStackTraceString CrashInfo("Code generation");
> +llvm::TimeTraceScope TimeScope("BackendCodegen", StringRef(""));
>  CodeGenPasses.run(*TheModule);
>}
> ```
> 
> But that actually brings me to another question - what about 
> `PrettyStackTraceString CrashInfo("Per-function optimization");`?
> Should that be wrapped into some section, too?
> I'm less certain here.
Ok, you've talked about `Timer CodeGenerationTime`, which corresponds to 
`Backend` scope.
As for this `BackendCodegen`, adding this I would prefer to add adjacent 
`"Per-function optimization"` and `"Per-module optimization passes"` together.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675



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

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 221568.
Tyker marked 7 inline comments as done.
Tyker added a comment.

fixed most comments


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

https://reviews.llvm.org/D63640

Files:
  clang/include/clang/AST/APValue.h
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/PrettyPrinter.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/AST/ast-dump-color.cpp
  clang/test/ASTMerge/APValue/APValue.cpp

Index: clang/test/ASTMerge/APValue/APValue.cpp
===
--- /dev/null
+++ clang/test/ASTMerge/APValue/APValue.cpp
@@ -0,0 +1,209 @@
+// RUN: %clang_cc1 -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -std=gnu++2a %s -DEMIT -ast-merge %t.pch -ast-dump-all | FileCheck %s
+
+#ifndef EMIT
+#define EMIT
+
+namespace Integer {
+
+constexpr int Unique_Int = int(6789);
+//CHECK:  VarDecl {{.*}} Unique_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'int' 6789
+
+constexpr __uint128_t Unique_Int128 = ((__uint128_t)0x75f17d6b3588f843 << 64) | 0xb13dea7c9c324e51;
+//CHECK:  VarDecl {{.*}} Unique_Int128 
+//CHECK-NEXT: ConstantExpr {{.*}} 'unsigned __int128' 156773562844924187900898496343692168785
+
+}
+
+namespace FloatingPoint {
+
+constexpr double Unique_Double = double(567890.67890);
+//CHECK:  VarDecl {{.*}} Unique_Double
+//CHECK-NEXT: ConstantExpr {{.*}} 'double' 5.678907e+05
+
+}
+
+// FIXME: Add test for FixedPoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
+namespace Struct {
+
+struct B {
+  int i;
+  double d;
+};
+
+constexpr B Basic_Struct = B{1, 0.7};
+//CHECK:  VarDecl {{.*}} Basic_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::B' {1, 7.00e-01}
+
+struct C {
+  int i = 9;
+};
+
+struct A : B {
+  int i;
+  double d;
+  C c;
+};
+
+constexpr A Advanced_Struct = A{Basic_Struct, 1, 79.789, {}};
+//CHECK:  VarDecl {{.*}} Advanced_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::A' {{[{][{]}}1, 7.00e-01}, 1, 7.978900e+01, {9}}
+
+}
+
+namespace Vector {
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Vector_Int = (v4si){8, 2, 3};
+//CHECK:  VarDecl {{.*}} Vector_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'Vector::v4si':'__attribute__((__vector_size__(4 * sizeof(int int' {8, 2, 3, 0}
+
+}
+
+namespace Array {
+
+constexpr int Array_Int[] = {1, 2, 3, 4, 5, 6};
+//CHECK:  VarDecl {{.*}} Array_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int [6]' {1, 2, 3, 4, 5, 6}
+
+struct A {
+  int i = 789;
+  double d = 67890.09876;
+};
+
+constexpr A Array2_Struct[][3] = {{{}, {-45678, 9.8}, {9}}, {{}}};
+//CHECK:  VarDecl {{.*}} Array2_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'Array::A const [2][3]' {{[{][{]}}{789, 6.789010e+04}, {-45678, 9.80e+00}, {9, 6.789010e+04}}, {{[{][{]}}789, 6.789010e+04}, {789, 6.789010e+04}, {789, 6.789010e+04}}}
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Array_Vector[] = {{1, 2, 3, 4}, {4, 5, 6, 7}};
+//CHECK:  VarDecl {{.*}} Array_Vector
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Array::v4si [2]' {{[{][{]}}1, 2, 3, 4}, {4, 5, 6, 7}}
+
+}
+
+namespace Union {
+
+struct A {
+  int i = 6789;
+  float f = 987.9876;
+};
+
+union U {
+  int i;
+  A a{567890, 9876.5678f};
+};
+
+constexpr U Unique_Union1 = U{0};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.i = 0}
+
+constexpr U Unique_Union2 = U{};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.a = {567890, 9.876567e+03}}
+
+}
+
+namespace MemberPointer{
+
+struct A {
+  struct B {
+struct C {
+  struct D {
+struct E {
+  struct F {
+struct G {
+  int i;
+};
+  };
+};
+  };
+};
+  };
+};
+
+constexpr auto MemberPointer1 = &A::B::C::D::E::F::G::i;
+//CHECK:  VarDecl {{.*}} MemberPointer1
+//CHECK-NEXT: ConstantExpr {{.*}} 'int MemberPointer::A::B::C::D::E::F::G::*' &G::i
+
+struct A1 {
+  struct B1 {
+int f() const {
+  return 0;
+}
+  };
+
+};
+
+constexpr auto MemberPointer2 = &A1::B1::f;
+//CHECK:  VarDecl {{.*}} MemberPointer2
+//CHECK-NEXT: ConstantExpr {{.*}} 'int (MemberPointer::A1::B1::*)() const' &B1::f
+
+}
+
+namespace std {
+  struct type_info;
+};
+
+namespace LValue {
+
+constexpr int LValueInt = 0;
+constexpr const int& ConstIntRef = LValueInt;
+//CHECK:  VarDecl {{.*}} ConstIntRef
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int' lvalue &

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.



Comment at: clang/lib/AST/APValue.cpp:599
 Out << '[' << Path[I].getAsArrayIndex() << ']';
-ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+ElemTy = cast(ElemTy)->getElementType();
   }

aaron.ballman wrote:
> Are you sure this doesn't change behavior? See the implementation of 
> `ASTContext::getAsArrayType()`. Same question applies below.
i ran the test suite after the change it there wasn't any test failures. but 
the test on dumping APValue are probably not as thorough as we would like them 
to be.
from analysis of `ASTContext::getAsArrayType()` the only effect i see on the 
element type is de-sugaring and canonicalization which shouldn't affect 
correctness of the output.  de-sugaring requires the ASTContext but 
canonicalization doesn't.

i think the best way the have higher confidence is to ask rsmith what he thinks.



Comment at: clang/test/ASTMerge/APValue/APValue.cpp:28
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Are you planning to address this in this patch? Also, I think it's 
> > > FixedPoint and not FixePoint.
> > i don't intend to add them in this patch or subsequent patches. i don't 
> > know how to use the features that have these representations, i don't even 
> > know if they can be stored stored in that AST. so this is untested code.
> > that said theses representations aren't complex. the imporing for 
> > FixePoint, ComplexInt, ComplexFloat is a no-op and for AddrLabelDiff it is 
> > trivial. for serialization, I can put an llvm_unreachable to mark them as 
> > untested if you want ?
> I don't think `llvm_unreachable` makes a whole lot of sense unless the code 
> is truly unreachable because there's no way to get an AST with that 
> representation. By code inspection, the code looks reasonable but it does 
> make me a bit uncomfortable to adopt it without tests. I suppose the FIXME is 
> a reasonable compromise in this case, but if you have some spare cycles to 
> investigate ways to get those representations, it would be appreciated.
the reason i proposed `llvm_unreachable` was because it passes the tests and 
prevents future developer from depending on the code that depend on it assuming 
it works.


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

https://reviews.llvm.org/D63640



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

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Konstantin Belousov via Phabricator via cfe-commits]

kib added a comment.

In fact, can we have an option controlling this ?  Does it have anything to do 
with -malign-data gcc switch ?

We do want to be able to optionally generate code ABI-compatible with modern 
gcc, per user discretion.


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

https://reviews.llvm.org/D60748



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

[PATCH] D67935: Add `#pragma clang deprecated`, used to deprecate macros

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington planned changes to this revision.
erik.pilkington added a comment.

Marking as "planned changes" to get this out of review queues. Aaron Ballman 
wants to add "macro attributes", which would be a better way of solving this 
problem, and is going to ask the C and C++ committees if there is an appetite 
for such an idea. This is blocked on getting that answer.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67935



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