[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

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

The text was written mostly by Sam McCall, screenshots are mostly made
by me.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58710

Files:
  clang-tools-extra/docs/clangd/ApplyClangTidyFixInVSCode.gif
  clang-tools-extra/docs/clangd/ApplyFixInVSCode.gif
  clang-tools-extra/docs/clangd/CodeCompletionInEmacsCompanyMode.png
  clang-tools-extra/docs/clangd/CodeCompletionInSublimeText.png
  clang-tools-extra/docs/clangd/CodeCompletionInVSCode.png
  clang-tools-extra/docs/clangd/CodeCompletionInYCM.png
  
clang-tools-extra/docs/clangd/CodeCompletionInsertsNamespaceQualifiersInVSCode.gif
  clang-tools-extra/docs/clangd/Compiling.rst
  clang-tools-extra/docs/clangd/Contributing.rst
  clang-tools-extra/docs/clangd/DiagnosticsInEmacsEglot.png
  clang-tools-extra/docs/clangd/ErrorsInVSCode.png
  clang-tools-extra/docs/clangd/Extensions.rst
  clang-tools-extra/docs/clangd/Features.rst
  clang-tools-extra/docs/clangd/FindAllReferencesInVSCode.gif
  clang-tools-extra/docs/clangd/FormatSelectionInVSCode.gif
  clang-tools-extra/docs/clangd/GoToDefinitionInVSCode.gif
  clang-tools-extra/docs/clangd/Installation.rst
  clang-tools-extra/docs/clangd/NavigationWithBreadcrumbsInVSCode.gif
  clang-tools-extra/docs/clangd/OutlineInVSCode.png
  clang-tools-extra/docs/clangd/SignatureHelpInVSCode.gif
  clang-tools-extra/docs/clangd/index.rst

Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -1,180 +1,30 @@
-
-Clangd
-
-
-.. contents::
+==
+clangd
+==
 
 .. toctree::
:maxdepth: 1
 
-:program:`Clangd` is an implementation of the `Language Server Protocol
-`_ leveraging Clang.
-Clangd's goal is to provide language "smartness" features like code completion,
-find references, etc. for clients such as C/C++ Editors.
-
-Using Clangd
-==
-
-:program:`Clangd` is not meant to be used by C/C++ developers directly but
-rather from a client implementing the protocol. A client would be typically
-implemented in an IDE or an editor.
-
-At the moment, `Visual Studio Code `_ is mainly
-used in order to test :program:`Clangd` but more clients are likely to make
-use of :program:`Clangd` in the future as it matures and becomes a production
-quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `installing Clangd`_ or
-`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder and
-launch the extension.
-
-Installing Clangd
-==
-
-Packages are available for debian-based distributions, see the `LLVM packages
-page `_. :program:`Clangd` is included in the
-`clang-tools` package.
-However, it is a good idea to check your distribution's packaging system first
-as it might already be available.
-
-Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
-
-Building Clangd
-==
-
-You can follow the instructions for `building Clang
-`_ but "extra Clang tools" is **not**
-optional.
-
-Current Status
-==
-
-Many features could be implemented in :program:`Clangd`.
-Here is a list of features that could be useful with the status of whether or
-not they are already implemented in :program:`Clangd` and specified in the
-Language Server Protocol. Note that for some of the features, it is not clear
-whether or not they should be part of the Language Server Protocol, so those
-features might be eventually developed outside :program:`Clangd` or as an
-extension to the protocol.
-
-+-++--+
-| C/C++ Editor feature|  LSP   |  Clangd  |
-+=++==+
-| Formatting  | Yes|   Yes|
-+-++--+
-| Completion  | Yes|   Yes|
-+-++--+
-| Diagnostics | Yes|   Yes|
-+-++--+
-| Fix-its | Yes|   Yes|
-+-++--+
-| Go to Definition| Yes|   Yes|
-+-++--+
-| Signature Help  | Yes|   Yes|
-+-++--+
-| Documen

[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188521.
gribozavr marked 2 inline comments as done.
gribozavr added a comment.

Addressed review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58710

Files:
  clang-tools-extra/docs/clangd/ApplyClangTidyFixInVSCode.gif
  clang-tools-extra/docs/clangd/ApplyFixInVSCode.gif
  clang-tools-extra/docs/clangd/CodeCompletionInEmacsCompanyMode.png
  clang-tools-extra/docs/clangd/CodeCompletionInSublimeText.png
  clang-tools-extra/docs/clangd/CodeCompletionInVSCode.png
  clang-tools-extra/docs/clangd/CodeCompletionInYCM.png
  
clang-tools-extra/docs/clangd/CodeCompletionInsertsNamespaceQualifiersInVSCode.gif
  clang-tools-extra/docs/clangd/Compiling.rst
  clang-tools-extra/docs/clangd/Contributing.rst
  clang-tools-extra/docs/clangd/DiagnosticsInEmacsEglot.png
  clang-tools-extra/docs/clangd/ErrorsInVSCode.png
  clang-tools-extra/docs/clangd/Extensions.rst
  clang-tools-extra/docs/clangd/Features.rst
  clang-tools-extra/docs/clangd/FindAllReferencesInVSCode.gif
  clang-tools-extra/docs/clangd/FormatSelectionInVSCode.gif
  clang-tools-extra/docs/clangd/GoToDefinitionInVSCode.gif
  clang-tools-extra/docs/clangd/Installation.rst
  clang-tools-extra/docs/clangd/NavigationWithBreadcrumbsInVSCode.gif
  clang-tools-extra/docs/clangd/OutlineInVSCode.png
  clang-tools-extra/docs/clangd/SignatureHelpInVSCode.gif
  clang-tools-extra/docs/clangd/index.rst

Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -1,180 +1,30 @@
-
-Clangd
-
-
-.. contents::
+==
+clangd
+==
 
 .. toctree::
:maxdepth: 1
 
-:program:`Clangd` is an implementation of the `Language Server Protocol
-`_ leveraging Clang.
-Clangd's goal is to provide language "smartness" features like code completion,
-find references, etc. for clients such as C/C++ Editors.
-
-Using Clangd
-==
-
-:program:`Clangd` is not meant to be used by C/C++ developers directly but
-rather from a client implementing the protocol. A client would be typically
-implemented in an IDE or an editor.
-
-At the moment, `Visual Studio Code `_ is mainly
-used in order to test :program:`Clangd` but more clients are likely to make
-use of :program:`Clangd` in the future as it matures and becomes a production
-quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `installing Clangd`_ or
-`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder and
-launch the extension.
-
-Installing Clangd
-==
-
-Packages are available for debian-based distributions, see the `LLVM packages
-page `_. :program:`Clangd` is included in the
-`clang-tools` package.
-However, it is a good idea to check your distribution's packaging system first
-as it might already be available.
-
-Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
-
-Building Clangd
-==
-
-You can follow the instructions for `building Clang
-`_ but "extra Clang tools" is **not**
-optional.
-
-Current Status
-==
-
-Many features could be implemented in :program:`Clangd`.
-Here is a list of features that could be useful with the status of whether or
-not they are already implemented in :program:`Clangd` and specified in the
-Language Server Protocol. Note that for some of the features, it is not clear
-whether or not they should be part of the Language Server Protocol, so those
-features might be eventually developed outside :program:`Clangd` or as an
-extension to the protocol.
-
-+-++--+
-| C/C++ Editor feature|  LSP   |  Clangd  |
-+=++==+
-| Formatting  | Yes|   Yes|
-+-++--+
-| Completion  | Yes|   Yes|
-+-++--+
-| Diagnostics | Yes|   Yes|
-+-++--+
-| Fix-its | Yes|   Yes|
-+-++--+
-| Go to Definition| Yes|   Yes|
-+-++--+
-| Signature Help  | Yes|   Yes|
-+-++--+
-| Document Highlights | Yes|   Yes|
-+--

[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/docs/clangd/Installation.rst:360
+
+- Pass an experimental `-background-index` command line argument.  With
+  this feature enabled, clangd incrementally builds an index of projects

ilya-biryukov wrote:
> I'd avoid mentioning it at all, we didn't vet it for the 8 release (and I 
> believe we landed critical fixes **after** the release cut).
> We could add it back during `9.0` release.
Removed in this patch (that I plan to cherry-pick into the 8.0 release branch). 
 Will re-add in a separate patch, so that it goes into 9.0.



Comment at: clang-tools-extra/docs/clangd/index.rst:20
 
-- Passing experimental `-background-index` commandline argument, which will
-  incrementally build an index of projects that you work on and make use of 
that
-  in clangd automatically.
-- Generate an index file using `clangd-indexer
-  
`_
-  Afterwards you can pass generated index file to clangd using
-  `-index-file=/path/to/index_file`.  *Note that clangd-indexer isn't included
-  alongside clangd in the standard clang-tools package. You will likely have to
-  build from source to use this option*
+clangd is an implementation of the `Language Server Protocol
+`__ and can work with

ilya-biryukov wrote:
> The original "clangd is a **language server**" looked somewhat better.
> Having a link to the LSP website is a good bet, though.
Reworded to add the phrase "language server".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58710



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

[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/docs/clangd/Installation.rst:360
+
+- Pass an experimental `-background-index` command line argument.  With
+  this feature enabled, clangd incrementally builds an index of projects

gribozavr wrote:
> ilya-biryukov wrote:
> > I'd avoid mentioning it at all, we didn't vet it for the 8 release (and I 
> > believe we landed critical fixes **after** the release cut).
> > We could add it back during `9.0` release.
> Removed in this patch (that I plan to cherry-pick into the 8.0 release 
> branch).  Will re-add in a separate patch, so that it goes into 9.0.
Reverted back after offline discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58710



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

[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188527.
gribozavr added a comment.

.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58710

Files:
  clang-tools-extra/docs/clangd/ApplyClangTidyFixInVSCode.gif
  clang-tools-extra/docs/clangd/ApplyFixInVSCode.gif
  clang-tools-extra/docs/clangd/CodeCompletionInEmacsCompanyMode.png
  clang-tools-extra/docs/clangd/CodeCompletionInSublimeText.png
  clang-tools-extra/docs/clangd/CodeCompletionInVSCode.png
  clang-tools-extra/docs/clangd/CodeCompletionInYCM.png
  
clang-tools-extra/docs/clangd/CodeCompletionInsertsNamespaceQualifiersInVSCode.gif
  clang-tools-extra/docs/clangd/Compiling.rst
  clang-tools-extra/docs/clangd/Contributing.rst
  clang-tools-extra/docs/clangd/DiagnosticsInEmacsEglot.png
  clang-tools-extra/docs/clangd/ErrorsInVSCode.png
  clang-tools-extra/docs/clangd/Extensions.rst
  clang-tools-extra/docs/clangd/Features.rst
  clang-tools-extra/docs/clangd/FindAllReferencesInVSCode.gif
  clang-tools-extra/docs/clangd/FormatSelectionInVSCode.gif
  clang-tools-extra/docs/clangd/GoToDefinitionInVSCode.gif
  clang-tools-extra/docs/clangd/Installation.rst
  clang-tools-extra/docs/clangd/NavigationWithBreadcrumbsInVSCode.gif
  clang-tools-extra/docs/clangd/OutlineInVSCode.png
  clang-tools-extra/docs/clangd/SignatureHelpInVSCode.gif
  clang-tools-extra/docs/clangd/index.rst

Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -1,180 +1,30 @@
-
-Clangd
-
-
-.. contents::
+==
+clangd
+==
 
 .. toctree::
:maxdepth: 1
 
-:program:`Clangd` is an implementation of the `Language Server Protocol
-`_ leveraging Clang.
-Clangd's goal is to provide language "smartness" features like code completion,
-find references, etc. for clients such as C/C++ Editors.
-
-Using Clangd
-==
-
-:program:`Clangd` is not meant to be used by C/C++ developers directly but
-rather from a client implementing the protocol. A client would be typically
-implemented in an IDE or an editor.
-
-At the moment, `Visual Studio Code `_ is mainly
-used in order to test :program:`Clangd` but more clients are likely to make
-use of :program:`Clangd` in the future as it matures and becomes a production
-quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `installing Clangd`_ or
-`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder and
-launch the extension.
-
-Installing Clangd
-==
-
-Packages are available for debian-based distributions, see the `LLVM packages
-page `_. :program:`Clangd` is included in the
-`clang-tools` package.
-However, it is a good idea to check your distribution's packaging system first
-as it might already be available.
-
-Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
-
-Building Clangd
-==
-
-You can follow the instructions for `building Clang
-`_ but "extra Clang tools" is **not**
-optional.
-
-Current Status
-==
-
-Many features could be implemented in :program:`Clangd`.
-Here is a list of features that could be useful with the status of whether or
-not they are already implemented in :program:`Clangd` and specified in the
-Language Server Protocol. Note that for some of the features, it is not clear
-whether or not they should be part of the Language Server Protocol, so those
-features might be eventually developed outside :program:`Clangd` or as an
-extension to the protocol.
-
-+-++--+
-| C/C++ Editor feature|  LSP   |  Clangd  |
-+=++==+
-| Formatting  | Yes|   Yes|
-+-++--+
-| Completion  | Yes|   Yes|
-+-++--+
-| Diagnostics | Yes|   Yes|
-+-++--+
-| Fix-its | Yes|   Yes|
-+-++--+
-| Go to Definition| Yes|   Yes|
-+-++--+
-| Signature Help  | Yes|   Yes|
-+-++--+
-| Document Highlights | Yes|   Yes|
-+-++--+
-| Rename  

[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188531.
gribozavr added a comment.
Herald added a reviewer: serge-sans-paille.

Added custom CSS for the details tag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58710

Files:
  clang-tools-extra/docs/_static/clang-tools-extra-styles.css
  clang-tools-extra/docs/_templates/layout.html
  clang-tools-extra/docs/clangd/ApplyClangTidyFixInVSCode.gif
  clang-tools-extra/docs/clangd/ApplyFixInVSCode.gif
  clang-tools-extra/docs/clangd/CodeCompletionInEmacsCompanyMode.png
  clang-tools-extra/docs/clangd/CodeCompletionInSublimeText.png
  clang-tools-extra/docs/clangd/CodeCompletionInVSCode.png
  clang-tools-extra/docs/clangd/CodeCompletionInYCM.png
  
clang-tools-extra/docs/clangd/CodeCompletionInsertsNamespaceQualifiersInVSCode.gif
  clang-tools-extra/docs/clangd/Compiling.rst
  clang-tools-extra/docs/clangd/Contributing.rst
  clang-tools-extra/docs/clangd/DiagnosticsInEmacsEglot.png
  clang-tools-extra/docs/clangd/ErrorsInVSCode.png
  clang-tools-extra/docs/clangd/Extensions.rst
  clang-tools-extra/docs/clangd/Features.rst
  clang-tools-extra/docs/clangd/FindAllReferencesInVSCode.gif
  clang-tools-extra/docs/clangd/FormatSelectionInVSCode.gif
  clang-tools-extra/docs/clangd/GoToDefinitionInVSCode.gif
  clang-tools-extra/docs/clangd/Installation.rst
  clang-tools-extra/docs/clangd/NavigationWithBreadcrumbsInVSCode.gif
  clang-tools-extra/docs/clangd/OutlineInVSCode.png
  clang-tools-extra/docs/clangd/SignatureHelpInVSCode.gif
  clang-tools-extra/docs/clangd/index.rst
  clang-tools-extra/docs/conf.py

Index: clang-tools-extra/docs/conf.py
===
--- clang-tools-extra/docs/conf.py
+++ clang-tools-extra/docs/conf.py
@@ -121,7 +121,7 @@
 # Add any paths that contain custom static files (such as style sheets) here,
 # relative to this directory. They are copied after the builtin static files,
 # so a file named "default.css" will overwrite the builtin "default.css".
-html_static_path = []
+html_static_path = ['_static']
 
 # If not '', a 'Last updated on:' timestamp is inserted at every page bottom,
 # using the given strftime format.
Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -1,180 +1,30 @@
-
-Clangd
-
-
-.. contents::
+==
+clangd
+==
 
 .. toctree::
:maxdepth: 1
 
-:program:`Clangd` is an implementation of the `Language Server Protocol
-`_ leveraging Clang.
-Clangd's goal is to provide language "smartness" features like code completion,
-find references, etc. for clients such as C/C++ Editors.
-
-Using Clangd
-==
-
-:program:`Clangd` is not meant to be used by C/C++ developers directly but
-rather from a client implementing the protocol. A client would be typically
-implemented in an IDE or an editor.
-
-At the moment, `Visual Studio Code `_ is mainly
-used in order to test :program:`Clangd` but more clients are likely to make
-use of :program:`Clangd` in the future as it matures and becomes a production
-quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `installing Clangd`_ or
-`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder and
-launch the extension.
-
-Installing Clangd
-==
-
-Packages are available for debian-based distributions, see the `LLVM packages
-page `_. :program:`Clangd` is included in the
-`clang-tools` package.
-However, it is a good idea to check your distribution's packaging system first
-as it might already be available.
-
-Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
-
-Building Clangd
-==
-
-You can follow the instructions for `building Clang
-`_ but "extra Clang tools" is **not**
-optional.
-
-Current Status
-==
-
-Many features could be implemented in :program:`Clangd`.
-Here is a list of features that could be useful with the status of whether or
-not they are already implemented in :program:`Clangd` and specified in the
-Language Server Protocol. Note that for some of the features, it is not clear
-whether or not they should be part of the Language Server Protocol, so those
-features might be eventually developed outside :program:`Clangd` or as an
-extension to the protocol.
-
-+-++--+
-| C/C++ Editor feature|  LSP   |  Clangd  |
-+=++==+
-| Formatting  | Yes|   Yes|
-+-+

[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188534.
gribozavr added a comment.

Separated user and developer documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58710

Files:
  clang-tools-extra/docs/_static/clang-tools-extra-styles.css
  clang-tools-extra/docs/_templates/layout.html
  clang-tools-extra/docs/clangd/ApplyClangTidyFixInVSCode.gif
  clang-tools-extra/docs/clangd/ApplyFixInVSCode.gif
  clang-tools-extra/docs/clangd/CodeCompletionInEmacsCompanyMode.png
  clang-tools-extra/docs/clangd/CodeCompletionInSublimeText.png
  clang-tools-extra/docs/clangd/CodeCompletionInVSCode.png
  clang-tools-extra/docs/clangd/CodeCompletionInYCM.png
  
clang-tools-extra/docs/clangd/CodeCompletionInsertsNamespaceQualifiersInVSCode.gif
  clang-tools-extra/docs/clangd/Compiling.rst
  clang-tools-extra/docs/clangd/Contributing.rst
  clang-tools-extra/docs/clangd/DeveloperDocumentation.rst
  clang-tools-extra/docs/clangd/DiagnosticsInEmacsEglot.png
  clang-tools-extra/docs/clangd/ErrorsInVSCode.png
  clang-tools-extra/docs/clangd/Extensions.rst
  clang-tools-extra/docs/clangd/Features.rst
  clang-tools-extra/docs/clangd/FindAllReferencesInVSCode.gif
  clang-tools-extra/docs/clangd/FormatSelectionInVSCode.gif
  clang-tools-extra/docs/clangd/GoToDefinitionInVSCode.gif
  clang-tools-extra/docs/clangd/Installation.rst
  clang-tools-extra/docs/clangd/NavigationWithBreadcrumbsInVSCode.gif
  clang-tools-extra/docs/clangd/OutlineInVSCode.png
  clang-tools-extra/docs/clangd/SignatureHelpInVSCode.gif
  clang-tools-extra/docs/clangd/index.rst
  clang-tools-extra/docs/conf.py
  clang-tools-extra/docs/index.rst

Index: clang-tools-extra/docs/index.rst
===
--- clang-tools-extra/docs/index.rst
+++ clang-tools-extra/docs/index.rst
@@ -21,6 +21,7 @@
pp-trace
clang-rename
clangd/index
+   clangd/DeveloperDocumentation
clang-doc
 
 
Index: clang-tools-extra/docs/conf.py
===
--- clang-tools-extra/docs/conf.py
+++ clang-tools-extra/docs/conf.py
@@ -121,7 +121,7 @@
 # Add any paths that contain custom static files (such as style sheets) here,
 # relative to this directory. They are copied after the builtin static files,
 # so a file named "default.css" will overwrite the builtin "default.css".
-html_static_path = []
+html_static_path = ['_static']
 
 # If not '', a 'Last updated on:' timestamp is inserted at every page bottom,
 # using the given strftime format.
Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -1,180 +1,27 @@
-
-Clangd
-
-
-.. contents::
+==
+clangd
+==
 
 .. toctree::
:maxdepth: 1
 
-:program:`Clangd` is an implementation of the `Language Server Protocol
-`_ leveraging Clang.
-Clangd's goal is to provide language "smartness" features like code completion,
-find references, etc. for clients such as C/C++ Editors.
-
-Using Clangd
-==
-
-:program:`Clangd` is not meant to be used by C/C++ developers directly but
-rather from a client implementing the protocol. A client would be typically
-implemented in an IDE or an editor.
-
-At the moment, `Visual Studio Code `_ is mainly
-used in order to test :program:`Clangd` but more clients are likely to make
-use of :program:`Clangd` in the future as it matures and becomes a production
-quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `installing Clangd`_ or
-`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder and
-launch the extension.
-
-Installing Clangd
-==
-
-Packages are available for debian-based distributions, see the `LLVM packages
-page `_. :program:`Clangd` is included in the
-`clang-tools` package.
-However, it is a good idea to check your distribution's packaging system first
-as it might already be available.
-
-Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
-
-Building Clangd
-==
-
-You can follow the instructions for `building Clang
-`_ but "extra Clang tools" is **not**
-optional.
-
-Current Status
-==
-
-Many features could be implemented in :program:`Clangd`.
-Here is a list of features that could be useful with the status of whether or
-not they are already implemented in :program:`Clangd` and specified in the
-Language Server Protocol. Note that for some of the features, it is not clear
-whether or not they should be part of the Language Server Protocol, so those
-features might be eventually developed outside :pr

[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188535.
gribozavr added a comment.

.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58710

Files:
  clang-tools-extra/docs/_static/clang-tools-extra-styles.css
  clang-tools-extra/docs/_templates/layout.html
  clang-tools-extra/docs/clangd/ApplyClangTidyFixInVSCode.gif
  clang-tools-extra/docs/clangd/ApplyFixInVSCode.gif
  clang-tools-extra/docs/clangd/CodeCompletionInEmacsCompanyMode.png
  clang-tools-extra/docs/clangd/CodeCompletionInSublimeText.png
  clang-tools-extra/docs/clangd/CodeCompletionInVSCode.png
  clang-tools-extra/docs/clangd/CodeCompletionInYCM.png
  
clang-tools-extra/docs/clangd/CodeCompletionInsertsNamespaceQualifiersInVSCode.gif
  clang-tools-extra/docs/clangd/DeveloperDocumentation.rst
  clang-tools-extra/docs/clangd/DiagnosticsInEmacsEglot.png
  clang-tools-extra/docs/clangd/ErrorsInVSCode.png
  clang-tools-extra/docs/clangd/Extensions.rst
  clang-tools-extra/docs/clangd/Features.rst
  clang-tools-extra/docs/clangd/FindAllReferencesInVSCode.gif
  clang-tools-extra/docs/clangd/FormatSelectionInVSCode.gif
  clang-tools-extra/docs/clangd/GoToDefinitionInVSCode.gif
  clang-tools-extra/docs/clangd/Installation.rst
  clang-tools-extra/docs/clangd/NavigationWithBreadcrumbsInVSCode.gif
  clang-tools-extra/docs/clangd/OutlineInVSCode.png
  clang-tools-extra/docs/clangd/SignatureHelpInVSCode.gif
  clang-tools-extra/docs/clangd/index.rst
  clang-tools-extra/docs/conf.py
  clang-tools-extra/docs/index.rst

Index: clang-tools-extra/docs/index.rst
===
--- clang-tools-extra/docs/index.rst
+++ clang-tools-extra/docs/index.rst
@@ -21,6 +21,7 @@
pp-trace
clang-rename
clangd/index
+   clangd/DeveloperDocumentation
clang-doc
 
 
Index: clang-tools-extra/docs/conf.py
===
--- clang-tools-extra/docs/conf.py
+++ clang-tools-extra/docs/conf.py
@@ -121,7 +121,7 @@
 # Add any paths that contain custom static files (such as style sheets) here,
 # relative to this directory. They are copied after the builtin static files,
 # so a file named "default.css" will overwrite the builtin "default.css".
-html_static_path = []
+html_static_path = ['_static']
 
 # If not '', a 'Last updated on:' timestamp is inserted at every page bottom,
 # using the given strftime format.
Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -1,180 +1,27 @@
-
-Clangd
-
-
-.. contents::
+==
+clangd
+==
 
 .. toctree::
:maxdepth: 1
 
-:program:`Clangd` is an implementation of the `Language Server Protocol
-`_ leveraging Clang.
-Clangd's goal is to provide language "smartness" features like code completion,
-find references, etc. for clients such as C/C++ Editors.
-
-Using Clangd
-==
-
-:program:`Clangd` is not meant to be used by C/C++ developers directly but
-rather from a client implementing the protocol. A client would be typically
-implemented in an IDE or an editor.
-
-At the moment, `Visual Studio Code `_ is mainly
-used in order to test :program:`Clangd` but more clients are likely to make
-use of :program:`Clangd` in the future as it matures and becomes a production
-quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `installing Clangd`_ or
-`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder and
-launch the extension.
-
-Installing Clangd
-==
-
-Packages are available for debian-based distributions, see the `LLVM packages
-page `_. :program:`Clangd` is included in the
-`clang-tools` package.
-However, it is a good idea to check your distribution's packaging system first
-as it might already be available.
-
-Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
-
-Building Clangd
-==
-
-You can follow the instructions for `building Clang
-`_ but "extra Clang tools" is **not**
-optional.
-
-Current Status
-==
-
-Many features could be implemented in :program:`Clangd`.
-Here is a list of features that could be useful with the status of whether or
-not they are already implemented in :program:`Clangd` and specified in the
-Language Server Protocol. Note that for some of the features, it is not clear
-whether or not they should be part of the Language Server Protocol, so those
-features might be eventually developed outside :program:`Clangd` or as an
-extension to the protocol.
-
-+-++--+
-| C/C++ Editor f

[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188536.
gribozavr added a comment.

Removed mentions of clangd v9.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58710

Files:
  clang-tools-extra/docs/_static/clang-tools-extra-styles.css
  clang-tools-extra/docs/_templates/layout.html
  clang-tools-extra/docs/clangd/ApplyClangTidyFixInVSCode.gif
  clang-tools-extra/docs/clangd/ApplyFixInVSCode.gif
  clang-tools-extra/docs/clangd/CodeCompletionInEmacsCompanyMode.png
  clang-tools-extra/docs/clangd/CodeCompletionInSublimeText.png
  clang-tools-extra/docs/clangd/CodeCompletionInVSCode.png
  clang-tools-extra/docs/clangd/CodeCompletionInYCM.png
  
clang-tools-extra/docs/clangd/CodeCompletionInsertsNamespaceQualifiersInVSCode.gif
  clang-tools-extra/docs/clangd/DeveloperDocumentation.rst
  clang-tools-extra/docs/clangd/DiagnosticsInEmacsEglot.png
  clang-tools-extra/docs/clangd/ErrorsInVSCode.png
  clang-tools-extra/docs/clangd/Extensions.rst
  clang-tools-extra/docs/clangd/Features.rst
  clang-tools-extra/docs/clangd/FindAllReferencesInVSCode.gif
  clang-tools-extra/docs/clangd/FormatSelectionInVSCode.gif
  clang-tools-extra/docs/clangd/GoToDefinitionInVSCode.gif
  clang-tools-extra/docs/clangd/Installation.rst
  clang-tools-extra/docs/clangd/NavigationWithBreadcrumbsInVSCode.gif
  clang-tools-extra/docs/clangd/OutlineInVSCode.png
  clang-tools-extra/docs/clangd/SignatureHelpInVSCode.gif
  clang-tools-extra/docs/clangd/index.rst
  clang-tools-extra/docs/conf.py
  clang-tools-extra/docs/index.rst

Index: clang-tools-extra/docs/index.rst
===
--- clang-tools-extra/docs/index.rst
+++ clang-tools-extra/docs/index.rst
@@ -21,6 +21,7 @@
pp-trace
clang-rename
clangd/index
+   clangd/DeveloperDocumentation
clang-doc
 
 
Index: clang-tools-extra/docs/conf.py
===
--- clang-tools-extra/docs/conf.py
+++ clang-tools-extra/docs/conf.py
@@ -121,7 +121,7 @@
 # Add any paths that contain custom static files (such as style sheets) here,
 # relative to this directory. They are copied after the builtin static files,
 # so a file named "default.css" will overwrite the builtin "default.css".
-html_static_path = []
+html_static_path = ['_static']
 
 # If not '', a 'Last updated on:' timestamp is inserted at every page bottom,
 # using the given strftime format.
Index: clang-tools-extra/docs/clangd/index.rst
===
--- clang-tools-extra/docs/clangd/index.rst
+++ clang-tools-extra/docs/clangd/index.rst
@@ -1,180 +1,27 @@
-
-Clangd
-
-
-.. contents::
+==
+clangd
+==
 
 .. toctree::
:maxdepth: 1
 
-:program:`Clangd` is an implementation of the `Language Server Protocol
-`_ leveraging Clang.
-Clangd's goal is to provide language "smartness" features like code completion,
-find references, etc. for clients such as C/C++ Editors.
-
-Using Clangd
-==
-
-:program:`Clangd` is not meant to be used by C/C++ developers directly but
-rather from a client implementing the protocol. A client would be typically
-implemented in an IDE or an editor.
-
-At the moment, `Visual Studio Code `_ is mainly
-used in order to test :program:`Clangd` but more clients are likely to make
-use of :program:`Clangd` in the future as it matures and becomes a production
-quality tool. If you are interested in trying :program:`Clangd` in combination
-with Visual Studio Code, you can start by `installing Clangd`_ or
-`building Clangd`_, then open Visual Studio Code in the clangd-vscode folder and
-launch the extension.
-
-Installing Clangd
-==
-
-Packages are available for debian-based distributions, see the `LLVM packages
-page `_. :program:`Clangd` is included in the
-`clang-tools` package.
-However, it is a good idea to check your distribution's packaging system first
-as it might already be available.
-
-Otherwise, you can install :program:`Clangd` by `building Clangd`_ first.
-
-Building Clangd
-==
-
-You can follow the instructions for `building Clang
-`_ but "extra Clang tools" is **not**
-optional.
-
-Current Status
-==
-
-Many features could be implemented in :program:`Clangd`.
-Here is a list of features that could be useful with the status of whether or
-not they are already implemented in :program:`Clangd` and specified in the
-Language Server Protocol. Note that for some of the features, it is not clear
-whether or not they should be part of the Language Server Protocol, so those
-features might be eventually developed outside :program:`Clangd` or as an
-extension to the protocol.
-
-+-++

[PATCH] D58710: Added more detailed documentation for clangd

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354992: Added more detailed documentation for clangd 
(authored by gribozavr, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58710?vs=188536&id=188539#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58710

Files:
  clang-tools-extra/trunk/docs/_static/clang-tools-extra-styles.css
  clang-tools-extra/trunk/docs/_templates/layout.html
  clang-tools-extra/trunk/docs/clangd/ApplyClangTidyFixInVSCode.gif
  clang-tools-extra/trunk/docs/clangd/ApplyFixInVSCode.gif
  clang-tools-extra/trunk/docs/clangd/CodeCompletionInEmacsCompanyMode.png
  clang-tools-extra/trunk/docs/clangd/CodeCompletionInSublimeText.png
  clang-tools-extra/trunk/docs/clangd/CodeCompletionInVSCode.png
  clang-tools-extra/trunk/docs/clangd/CodeCompletionInYCM.png
  
clang-tools-extra/trunk/docs/clangd/CodeCompletionInsertsNamespaceQualifiersInVSCode.gif
  clang-tools-extra/trunk/docs/clangd/DeveloperDocumentation.rst
  clang-tools-extra/trunk/docs/clangd/DiagnosticsInEmacsEglot.png
  clang-tools-extra/trunk/docs/clangd/ErrorsInVSCode.png
  clang-tools-extra/trunk/docs/clangd/Extensions.rst
  clang-tools-extra/trunk/docs/clangd/Features.rst
  clang-tools-extra/trunk/docs/clangd/FindAllReferencesInVSCode.gif
  clang-tools-extra/trunk/docs/clangd/FormatSelectionInVSCode.gif
  clang-tools-extra/trunk/docs/clangd/GoToDefinitionInVSCode.gif
  clang-tools-extra/trunk/docs/clangd/Installation.rst
  clang-tools-extra/trunk/docs/clangd/NavigationWithBreadcrumbsInVSCode.gif
  clang-tools-extra/trunk/docs/clangd/OutlineInVSCode.png
  clang-tools-extra/trunk/docs/clangd/SignatureHelpInVSCode.gif
  clang-tools-extra/trunk/docs/clangd/index.rst
  clang-tools-extra/trunk/docs/conf.py
  clang-tools-extra/trunk/docs/index.rst

Index: clang-tools-extra/trunk/docs/_templates/layout.html
===
--- clang-tools-extra/trunk/docs/_templates/layout.html
+++ clang-tools-extra/trunk/docs/_templates/layout.html
@@ -0,0 +1,3 @@
+{% extends "!layout.html" %}
+
+{% set css_files = css_files + ['_static/clang-tools-extra-styles.css'] %}
Index: clang-tools-extra/trunk/docs/clangd/Installation.rst
===
--- clang-tools-extra/trunk/docs/clangd/Installation.rst
+++ clang-tools-extra/trunk/docs/clangd/Installation.rst
@@ -0,0 +1,369 @@
+===
+Getting started with clangd
+===
+
+.. role:: raw-html(raw)
+   :format: html
+
+To use clangd, you need to:
+
+- install clangd,
+- install a plugin for your editor,
+- tell clangd how your project is built.
+
+Installing clangd
+=
+
+You need a **recent** version of clangd: 7.0 was the first usable release, and
+8.0 is much better.
+
+After installing, ``clangd --version`` should print ``clangd version 7.0.0`` or
+later.
+
+:raw-html:`macOS`
+
+`Homebrew `__ can install clangd along with LLVM:
+
+.. code-block:: console
+
+  $ brew install llvm
+
+If you don't want to use Homebrew, you can download the a binary release of
+LLVM from `releases.llvm.org `__.
+Alongside ``bin/clangd`` you will need at least ``lib/clang/*/include``:
+
+.. code-block:: console
+
+  $ cp clang+llvm-7.0.0/bin/clangd /usr/local/bin/clangd
+  $ cp -r clang+llvm-7.0.0/lib/clang/ /usr/local/lib/
+
+:raw-html:``
+
+:raw-html:`Windows`
+
+Download and run the LLVM installer from `releases.llvm.org
+`__.
+
+:raw-html:``
+
+:raw-html:`Debian/Ubuntu`
+
+The ``clang-tools`` package usually contains an old version of clangd.
+
+Try to install the latest release (8.0):
+
+.. code-block:: console
+
+  $ sudo apt-get install clang-tools-8
+
+If that is not found, at least ``clang-tools-7`` should be available.
+
+The ``clangd`` executable will be installed as ``/usr/bin/clangd-8``. Make it
+the default ``clangd``:
+
+.. code-block:: console
+
+  $ sudo update-alternatives --install /usr/bin/clangd clangd /usr/bin/clangd-8 100
+
+:raw-html:``
+
+:raw-html:`Other systems`
+
+Most distributions include clangd in a ``clang-tools`` package, or in the full
+``llvm`` distribution.
+
+For some platforms, binaries are also avaliable at `releases.llvm.org
+`__.
+
+:raw-html:``
+
+Editor plugins
+==
+
+Language Server plugins are available for many editors. In principle, clangd
+should work with any of them, though the feature set and UI may vary.
+
+Here are some plugins we know work well with clangd.
+
+:raw-html:`YouCompleteMe for Vim`
+
+`YouCompleteMe `__ supports clangd.
+However, clangd support is not turned on by default, so you must install
+YouCompleteMe 

[PATCH] D58717: Added documentation for clangd v9+ features

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
gribozavr added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous, 
MaskRay, ioeric.
Herald added a project: clang.
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58717

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


Index: clang-tools-extra/docs/clangd/Features.rst
===
--- clang-tools-extra/docs/clangd/Features.rst
+++ clang-tools-extra/docs/clangd/Features.rst
@@ -38,6 +38,30 @@
 
 :raw-html:``
 
+**(New in v9)**
+If a missing symbol was seen in a file you've edited recently, clangd will
+suggest inserting it.
+
+clang-tidy checks
+-
+
+**(New in v9)**
+clangd embeds `clang-tidy `__
+which provides extra hints about code problems: bug-prone patterns,
+performance traps, and style issues.
+
+:raw-html:`Animated demo`
+
+.. image:: ApplyClangTidyFixInVSCode.gif
+   :align: center
+   :alt: Applying a fix suggested by the compiler
+
+:raw-html:``
+
+clangd respects your project's ``.clang-tidy`` file which controls the checks
+to run. Not all checks work within clangd.  You must pass the ``-clang-tidy``
+flag to enable this feature.
+
 Code completion
 ===
 
@@ -92,6 +116,9 @@
 If there is no project-wide index, cross-references work across the files
 you have opened.
 
+**(New in v9)**
+clangd will also automatically index your whole project.
+
 Find definition/declaration
 ---
 
@@ -105,6 +132,13 @@
 
 :raw-html:``
 
+**(New in v9)**
+Some editors only expose "find definition"; use "find definition" on the
+definition to jump to the declaration.
+
+"Find definition" also works on ``#include`` lines, to jump to the included
+file.
+
 Find references
 ---
 


Index: clang-tools-extra/docs/clangd/Features.rst
===
--- clang-tools-extra/docs/clangd/Features.rst
+++ clang-tools-extra/docs/clangd/Features.rst
@@ -38,6 +38,30 @@
 
 :raw-html:``
 
+**(New in v9)**
+If a missing symbol was seen in a file you've edited recently, clangd will
+suggest inserting it.
+
+clang-tidy checks
+-
+
+**(New in v9)**
+clangd embeds `clang-tidy `__
+which provides extra hints about code problems: bug-prone patterns,
+performance traps, and style issues.
+
+:raw-html:`Animated demo`
+
+.. image:: ApplyClangTidyFixInVSCode.gif
+   :align: center
+   :alt: Applying a fix suggested by the compiler
+
+:raw-html:``
+
+clangd respects your project's ``.clang-tidy`` file which controls the checks
+to run. Not all checks work within clangd.  You must pass the ``-clang-tidy``
+flag to enable this feature.
+
 Code completion
 ===
 
@@ -92,6 +116,9 @@
 If there is no project-wide index, cross-references work across the files
 you have opened.
 
+**(New in v9)**
+clangd will also automatically index your whole project.
+
 Find definition/declaration
 ---
 
@@ -105,6 +132,13 @@
 
 :raw-html:``
 
+**(New in v9)**
+Some editors only expose "find definition"; use "find definition" on the
+definition to jump to the declaration.
+
+"Find definition" also works on ``#include`` lines, to jump to the included
+file.
+
 Find references
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58717: Added documentation for clangd v9+ features

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354994: Added documentation for clangd v9+ features 
(authored by gribozavr, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58717?vs=188542&id=188546#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58717

Files:
  clang-tools-extra/trunk/docs/clangd/Features.rst


Index: clang-tools-extra/trunk/docs/clangd/Features.rst
===
--- clang-tools-extra/trunk/docs/clangd/Features.rst
+++ clang-tools-extra/trunk/docs/clangd/Features.rst
@@ -38,6 +38,30 @@
 
 :raw-html:``
 
+**(New in v9)**
+If a missing symbol was seen in a file you've edited recently, clangd will
+suggest inserting it.
+
+clang-tidy checks
+-
+
+**(New in v9)**
+clangd embeds `clang-tidy `__
+which provides extra hints about code problems: bug-prone patterns,
+performance traps, and style issues.
+
+:raw-html:`Animated demo`
+
+.. image:: ApplyClangTidyFixInVSCode.gif
+   :align: center
+   :alt: Applying a fix suggested by the compiler
+
+:raw-html:``
+
+clangd respects your project's ``.clang-tidy`` file which controls the checks
+to run. Not all checks work within clangd.  You must pass the ``-clang-tidy``
+flag to enable this feature.
+
 Code completion
 ===
 
@@ -92,6 +116,9 @@
 If there is no project-wide index, cross-references work across the files
 you have opened.
 
+**(New in v9)**
+clangd will also automatically index your whole project.
+
 Find definition/declaration
 ---
 
@@ -105,6 +132,13 @@
 
 :raw-html:``
 
+**(New in v9)**
+Some editors only expose "find definition"; use "find definition" on the
+definition to jump to the declaration.
+
+"Find definition" also works on ``#include`` lines, to jump to the included
+file.
+
 Find references
 ---
 


Index: clang-tools-extra/trunk/docs/clangd/Features.rst
===
--- clang-tools-extra/trunk/docs/clangd/Features.rst
+++ clang-tools-extra/trunk/docs/clangd/Features.rst
@@ -38,6 +38,30 @@
 
 :raw-html:``
 
+**(New in v9)**
+If a missing symbol was seen in a file you've edited recently, clangd will
+suggest inserting it.
+
+clang-tidy checks
+-
+
+**(New in v9)**
+clangd embeds `clang-tidy `__
+which provides extra hints about code problems: bug-prone patterns,
+performance traps, and style issues.
+
+:raw-html:`Animated demo`
+
+.. image:: ApplyClangTidyFixInVSCode.gif
+   :align: center
+   :alt: Applying a fix suggested by the compiler
+
+:raw-html:``
+
+clangd respects your project's ``.clang-tidy`` file which controls the checks
+to run. Not all checks work within clangd.  You must pass the ``-clang-tidy``
+flag to enable this feature.
+
 Code completion
 ===
 
@@ -92,6 +116,9 @@
 If there is no project-wide index, cross-references work across the files
 you have opened.
 
+**(New in v9)**
+clangd will also automatically index your whole project.
+
 Find definition/declaration
 ---
 
@@ -105,6 +132,13 @@
 
 :raw-html:``
 
+**(New in v9)**
+Some editors only expose "find definition"; use "find definition" on the
+definition to jump to the declaration.
+
+"Find definition" also works on ``#include`` lines, to jump to the included
+file.
+
 Find references
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58721: Added release notes for clangd 8

[Dmitri Gribenko via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58721

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clangd/Extensions.rst
  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
@@ -350,6 +350,8 @@
 Creating this file by hand is a reasonable place to start if your project is
 quite simple.
 
+.. _project-wide-index:
+
 Project-wide Index
 ==
 
Index: clang-tools-extra/docs/clangd/Extensions.rst
===
--- clang-tools-extra/docs/clangd/Extensions.rst
+++ clang-tools-extra/docs/clangd/Extensions.rst
@@ -38,6 +38,8 @@
 
 If the corresponding file can't be determined, ``""`` is returned.
 
+.. _lsp-extension-file-status:
+
 File status
 ===
 
@@ -64,6 +66,8 @@
 
 Enables receiving ``textDocument/clangd.fileStatus`` notifications.
 
+.. _lsp-extension-compilation-commands:
+
 Compilation commands
 
 
@@ -133,6 +137,8 @@
 
 Requests that clangd send ``Diagnostic.category``.
 
+.. _lsp-extension-code-actions-in-diagnostics:
+
 Inline fixes for diagnostics
 
 
@@ -150,6 +156,8 @@
 
 Requests clangd to send ``Diagnostic.codeActions``.
 
+.. _lsp-extension-symbol-info:
+
 Symbol info request
 ===
 
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -32,7 +32,57 @@
 Improvements to clangd
 --
 
-The improvements are...
+- clangd now adds namespace qualifiers in code completion, for example, if you
+  type "``vec``", the list of completions will include "``std::vector``".
+
+  See also: `r343248 `__.
+
+- When a :ref:`global index ` is available, clangd will use it to augment the
+  results of "go to definition" and "find references" queries. Global index
+  also enables global code completion, which suggests symbols that are not
+  imported in the current file and automatically inserts the missing
+  ``#include`` directives.
+
+- clangd stores the symbol index on disk in a new compact binary serialization
+  format.  It is 10x more compact than YAML and 40% more compact than gzipped
+  YAML.
+
+  See also: `r341375 `__.
+
+- clangd has a new efficient symbol index suitable for complex and fuzzy
+  queries and large code bases (e.g., LLVM, Chromium).  This index is used for
+  code completion, go to definition, and cross-references.  The architecture of
+  the index allows for complex and fuzzy retrieval criteria and sophisticated
+  scoring.
+
+  See also: `discussion on the mailing list
+  `__, `design
+  doc
+  `__.
+
+- clangd has a new LSP extension that communicates information about activity
+  on clangd's per-file worker thread.  This information can be displayed to
+  users to let them know that the language server is busy with something.  For
+  example, in clangd, building the AST blocks many other operations.
+
+  More info: :ref:`lsp-extension-file-status`.
+
+- clangd has a new LSP extension that allows the client to supply the
+  compilation commands over LSP, instead of finding compile_commands.json on
+  disk.
+
+  More info: :ref:`lsp-extension-compilation-commands`.
+
+- clangd has a new LSP extension that allows the client to request code actions
+  to be sent together with diagnostics, instead of asynchronously.
+
+  More info: :ref:`lsp-extension-code-actions-in-diagnostics`.
+
+- clangd has a new LSP extension that allows the client to resolve a symbol in
+  a light-weight manner, without retrieving further information (like
+  definition location, which may require consulting an index).
+
+  More info: :ref:`lsp-extension-symbol-info`.
 
 
 Improvements to clang-query
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58721: Added release notes for clangd 8

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188555.
gribozavr added a comment.

.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58721

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clangd/Extensions.rst
  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
@@ -350,6 +350,8 @@
 Creating this file by hand is a reasonable place to start if your project is
 quite simple.
 
+.. _project-wide-index:
+
 Project-wide Index
 ==
 
Index: clang-tools-extra/docs/clangd/Extensions.rst
===
--- clang-tools-extra/docs/clangd/Extensions.rst
+++ clang-tools-extra/docs/clangd/Extensions.rst
@@ -38,6 +38,8 @@
 
 If the corresponding file can't be determined, ``""`` is returned.
 
+.. _lsp-extension-file-status:
+
 File status
 ===
 
@@ -64,6 +66,8 @@
 
 Enables receiving ``textDocument/clangd.fileStatus`` notifications.
 
+.. _lsp-extension-compilation-commands:
+
 Compilation commands
 
 
@@ -133,6 +137,8 @@
 
 Requests that clangd send ``Diagnostic.category``.
 
+.. _lsp-extension-code-actions-in-diagnostics:
+
 Inline fixes for diagnostics
 
 
@@ -150,6 +156,8 @@
 
 Requests clangd to send ``Diagnostic.codeActions``.
 
+.. _lsp-extension-symbol-info:
+
 Symbol info request
 ===
 
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -32,7 +32,57 @@
 Improvements to clangd
 --
 
-The improvements are...
+- clangd now adds namespace qualifiers in code completion, for example, if you
+  type "``vec``", the list of completions will include "``std::vector``".
+
+  See also: `r343248 `__.
+
+- When a :ref:`global index ` is available, clangd will use it to augment the
+  results of "go to definition" and "find references" queries. Global index
+  also enables global code completion, which suggests symbols that are not
+  imported in the current file and automatically inserts the missing
+  ``#include`` directives.
+
+- clangd stores the symbol index on disk in a new compact binary serialization
+  format.  It is 10x more compact than YAML and 40% more compact than gzipped
+  YAML.
+
+  See also: `r341375 `__.
+
+- clangd has a new efficient symbol index suitable for complex and fuzzy
+  queries and large code bases (e.g., LLVM, Chromium).  This index is used for
+  code completion, go to definition, and cross-references.  The architecture of
+  the index allows for complex and fuzzy retrieval criteria and sophisticated
+  scoring.
+
+  See also: `discussion on the mailing list
+  `__, `design
+  doc
+  `__.
+
+- clangd has a new LSP extension that communicates information about activity
+  on clangd's per-file worker thread.  This information can be displayed to
+  users to let them know that the language server is busy with something.  For
+  example, in clangd, building the AST blocks many other operations.
+
+  More info: :ref:`lsp-extension-file-status`.
+
+- clangd has a new LSP extension that allows the client to supply the
+  compilation commands over LSP, instead of finding compile_commands.json on
+  disk.
+
+  More info: :ref:`lsp-extension-compilation-commands`.
+
+- clangd has a new LSP extension that allows the client to request fixes to be
+  sent together with diagnostics, instead of asynchronously.
+
+  More info: :ref:`lsp-extension-code-actions-in-diagnostics`.
+
+- clangd has a new LSP extension that allows the client to resolve a symbol in
+  a light-weight manner, without retrieving further information (like
+  definition location, which may require consulting an index).
+
+  More info: :ref:`lsp-extension-symbol-info`.
 
 
 Improvements to clang-query
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58768: Moved SymbolLocation into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
gribozavr added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, mgorny.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58768

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/Serialization.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolLocation.cpp
  clang-tools-extra/clangd/index/SymbolLocation.h
  clang-tools-extra/clangd/index/YAMLSerialization.cpp

Index: clang-tools-extra/clangd/index/YAMLSerialization.cpp
===
--- clang-tools-extra/clangd/index/YAMLSerialization.cpp
+++ clang-tools-extra/clangd/index/YAMLSerialization.cpp
@@ -14,6 +14,7 @@
 
 #include "Index.h"
 #include "Serialization.h"
+#include "SymbolLocation.h"
 #include "Trace.h"
 #include "dex/Dex.h"
 #include "llvm/ADT/Optional.h"
Index: clang-tools-extra/clangd/index/SymbolLocation.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/SymbolLocation.h
@@ -0,0 +1,88 @@
+//===--- SymbolLocation.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_INDEX_SYMBOL_LOCATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_LOCATION_H
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+
+namespace clang {
+namespace clangd {
+
+struct SymbolLocation {
+  // Specify a position (Line, Column) of symbol. Using Line/Column allows us to
+  // build LSP responses without reading the file content.
+  //
+  // Position is encoded into 32 bits to save space.
+  // If Line/Column overflow, the value will be their maximum value.
+  struct Position {
+Position() : Line(0), Column(0) {}
+void setLine(uint32_t Line);
+uint32_t line() const { return Line; }
+void setColumn(uint32_t Column);
+uint32_t column() const { return Column; }
+
+bool hasOverflow() const {
+  return Line >= MaxLine || Column >= MaxColumn;
+}
+
+static constexpr uint32_t MaxLine = (1 << 20) - 1;
+static constexpr uint32_t MaxColumn = (1 << 12) - 1;
+
+  private:
+uint32_t Line : 20; // 0-based
+// Using UTF-16 code units.
+uint32_t Column : 12; // 0-based
+  };
+
+  /// The symbol range, using half-open range [Start, End).
+  Position Start;
+  Position End;
+
+  explicit operator bool() const { return !llvm::StringRef(FileURI).empty(); }
+
+  // The URI of the source file where a symbol occurs.
+  // The string must be null-terminated.
+  //
+  // We avoid using llvm::StringRef here to save memory.
+  // WARNING: unless you know what you are doing, it is recommended to use it
+  // via llvm::StringRef.
+  const char *FileURI = "";
+};
+
+inline bool operator==(const SymbolLocation::Position &L,
+   const SymbolLocation::Position &R) {
+  return std::make_tuple(L.line(), L.column()) ==
+ std::make_tuple(R.line(), R.column());
+}
+inline bool operator<(const SymbolLocation::Position &L,
+  const SymbolLocation::Position &R) {
+  return std::make_tuple(L.line(), L.column()) <
+ std::make_tuple(R.line(), R.column());
+}
+inline bool operator==(const SymbolLocation &L, const SymbolLocation &R) {
+  assert(L.FileURI && R.FileURI);
+  return !std::strcmp(L.FileURI, R.FileURI) &&
+ std::tie(L.Start, L.End) == std::tie(R.Start, R.End);
+}
+inline bool operator<(const SymbolLocation &L, const SymbolLocation &R) {
+  assert(L.FileURI && R.FileURI);
+  int Cmp = std::strcmp(L.FileURI, R.FileURI);
+  if (Cmp != 0)
+return Cmp < 0;
+  return std::tie(L.Start, L.End) < std::tie(R.Start, R.End);
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolLocation &);
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_LOCATION_H
Index: clang-tools-extra/clangd/index/SymbolLocation.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/index/SymbolLocation.cpp
@@ -0,0 +1,40 @@
+//===--- SymbolLocation.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
+//
+

[PATCH] D58769: Moved DenseMap support for SymbolID into SymbolID.h

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
gribozavr added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58769

Files:
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/SymbolID.h


Index: clang-tools-extra/clangd/index/SymbolID.h
===
--- clang-tools-extra/clangd/index/SymbolID.h
+++ clang-tools-extra/clangd/index/SymbolID.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
 
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -61,4 +62,25 @@
 } // namespace clangd
 } // namespace clang
 
+namespace llvm {
+// Support SymbolIDs as DenseMap keys.
+template <> struct DenseMapInfo {
+  static inline clang::clangd::SymbolID getEmptyKey() {
+static clang::clangd::SymbolID EmptyKey("EMPTYKEY");
+return EmptyKey;
+  }
+  static inline clang::clangd::SymbolID getTombstoneKey() {
+static clang::clangd::SymbolID TombstoneKey("TOMBSTONEKEY");
+return TombstoneKey;
+  }
+  static unsigned getHashValue(const clang::clangd::SymbolID &Sym) {
+return hash_value(Sym);
+  }
+  static bool isEqual(const clang::clangd::SymbolID &LHS,
+  const clang::clangd::SymbolID &RHS) {
+return LHS == RHS;
+  }
+};
+} // namespace llvm
+
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
Index: clang-tools-extra/clangd/index/Index.h
===
--- clang-tools-extra/clangd/index/Index.h
+++ clang-tools-extra/clangd/index/Index.h
@@ -94,31 +94,6 @@
 }
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolLocation &);
 
-} // namespace clangd
-} // namespace clang
-namespace llvm {
-// Support SymbolIDs as DenseMap keys.
-template <> struct DenseMapInfo {
-  static inline clang::clangd::SymbolID getEmptyKey() {
-static clang::clangd::SymbolID EmptyKey("EMPTYKEY");
-return EmptyKey;
-  }
-  static inline clang::clangd::SymbolID getTombstoneKey() {
-static clang::clangd::SymbolID TombstoneKey("TOMBSTONEKEY");
-return TombstoneKey;
-  }
-  static unsigned getHashValue(const clang::clangd::SymbolID &Sym) {
-return hash_value(Sym);
-  }
-  static bool isEqual(const clang::clangd::SymbolID &LHS,
-  const clang::clangd::SymbolID &RHS) {
-return LHS == RHS;
-  }
-};
-} // namespace llvm
-namespace clang {
-namespace clangd {
-
 // Describes the source of information about a symbol.
 // Mainly useful for debugging, e.g. understanding code completion reuslts.
 // This is a bitfield as information can be combined from several sources.


Index: clang-tools-extra/clangd/index/SymbolID.h
===
--- clang-tools-extra/clangd/index/SymbolID.h
+++ clang-tools-extra/clangd/index/SymbolID.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
 
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -61,4 +62,25 @@
 } // namespace clangd
 } // namespace clang
 
+namespace llvm {
+// Support SymbolIDs as DenseMap keys.
+template <> struct DenseMapInfo {
+  static inline clang::clangd::SymbolID getEmptyKey() {
+static clang::clangd::SymbolID EmptyKey("EMPTYKEY");
+return EmptyKey;
+  }
+  static inline clang::clangd::SymbolID getTombstoneKey() {
+static clang::clangd::SymbolID TombstoneKey("TOMBSTONEKEY");
+return TombstoneKey;
+  }
+  static unsigned getHashValue(const clang::clangd::SymbolID &Sym) {
+return hash_value(Sym);
+  }
+  static bool isEqual(const clang::clangd::SymbolID &LHS,
+  const clang::clangd::SymbolID &RHS) {
+return LHS == RHS;
+  }
+};
+} // namespace llvm
+
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
Index: clang-tools-extra/clangd/index/Index.h
===
--- clang-tools-extra/clangd/index/Index.h
+++ clang-tools-extra/clangd/index/Index.h
@@ -94,31 +94,6 @@
 }
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolLocation &);
 
-} // namespace clangd
-} // namespace clang
-namespace llvm {
-// Support SymbolIDs as DenseMap keys.
-template <> struct DenseMapInfo {
-  static inline clang::clangd::SymbolID getEmptyKey() {
-static clang::clangd::SymbolID EmptyKey("EMPTYKEY");
-return EmptyKey;
-  }
-  static inline clang::clangd::SymbolID getTombstoneKey() {
-static clang::clangd::SymbolID TombstoneKey("TOMBSTONEKEY");
-return TombstoneKey;
-  }
-  static unsigned getHashValue(const clang::clangd::SymbolID &Sym) {
-return hash_value(Sym);
-  }
-  static bool isEqual

[PATCH] D58768: Moved SymbolLocation into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL355082: Moved SymbolLocation into its own header and 
implementation file (authored by gribozavr, committed by ).
Herald added subscribers: llvm-commits, ilya-biryukov.
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D58768?vs=188695&id=188699#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58768

Files:
  clang-tools-extra/trunk/clangd/CMakeLists.txt
  clang-tools-extra/trunk/clangd/XRefs.cpp
  clang-tools-extra/trunk/clangd/index/Index.cpp
  clang-tools-extra/trunk/clangd/index/Index.h
  clang-tools-extra/trunk/clangd/index/Merge.cpp
  clang-tools-extra/trunk/clangd/index/Serialization.cpp
  clang-tools-extra/trunk/clangd/index/Serialization.h
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/index/SymbolLocation.cpp
  clang-tools-extra/trunk/clangd/index/SymbolLocation.h
  clang-tools-extra/trunk/clangd/index/YAMLSerialization.cpp

Index: clang-tools-extra/trunk/clangd/index/Index.cpp
===
--- clang-tools-extra/trunk/clangd/index/Index.cpp
+++ clang-tools-extra/trunk/clangd/index/Index.cpp
@@ -16,30 +16,6 @@
 namespace clang {
 namespace clangd {
 
-constexpr uint32_t SymbolLocation::Position::MaxLine;
-constexpr uint32_t SymbolLocation::Position::MaxColumn;
-void SymbolLocation::Position::setLine(uint32_t L) {
-  if (L > MaxLine) {
-Line = MaxLine;
-return;
-  }
-  Line = L;
-}
-void SymbolLocation::Position::setColumn(uint32_t Col) {
-  if (Col > MaxColumn) {
-Column = MaxColumn;
-return;
-  }
-  Column = Col;
-}
-
-llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const SymbolLocation &L) {
-  if (!L)
-return OS << "(none)";
-  return OS << L.FileURI << "[" << L.Start.line() << ":" << L.Start.column()
-<< "-" << L.End.line() << ":" << L.End.column() << ")";
-}
-
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, SymbolOrigin O) {
   if (O == SymbolOrigin::Unknown)
 return OS << "unknown";
Index: clang-tools-extra/trunk/clangd/index/SymbolLocation.h
===
--- clang-tools-extra/trunk/clangd/index/SymbolLocation.h
+++ clang-tools-extra/trunk/clangd/index/SymbolLocation.h
@@ -0,0 +1,88 @@
+//===--- SymbolLocation.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_INDEX_SYMBOL_LOCATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_LOCATION_H
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+
+namespace clang {
+namespace clangd {
+
+struct SymbolLocation {
+  // Specify a position (Line, Column) of symbol. Using Line/Column allows us to
+  // build LSP responses without reading the file content.
+  //
+  // Position is encoded into 32 bits to save space.
+  // If Line/Column overflow, the value will be their maximum value.
+  struct Position {
+Position() : Line(0), Column(0) {}
+void setLine(uint32_t Line);
+uint32_t line() const { return Line; }
+void setColumn(uint32_t Column);
+uint32_t column() const { return Column; }
+
+bool hasOverflow() const {
+  return Line >= MaxLine || Column >= MaxColumn;
+}
+
+static constexpr uint32_t MaxLine = (1 << 20) - 1;
+static constexpr uint32_t MaxColumn = (1 << 12) - 1;
+
+  private:
+uint32_t Line : 20; // 0-based
+// Using UTF-16 code units.
+uint32_t Column : 12; // 0-based
+  };
+
+  /// The symbol range, using half-open range [Start, End).
+  Position Start;
+  Position End;
+
+  explicit operator bool() const { return !llvm::StringRef(FileURI).empty(); }
+
+  // The URI of the source file where a symbol occurs.
+  // The string must be null-terminated.
+  //
+  // We avoid using llvm::StringRef here to save memory.
+  // WARNING: unless you know what you are doing, it is recommended to use it
+  // via llvm::StringRef.
+  const char *FileURI = "";
+};
+
+inline bool operator==(const SymbolLocation::Position &L,
+   const SymbolLocation::Position &R) {
+  return std::make_tuple(L.line(), L.column()) ==
+ std::make_tuple(R.line(), R.column());
+}
+inline bool operator<(const SymbolLocation::Position &L,
+  const SymbolLocation::Position &R) {
+  return std::make_tuple(L.line(), L.column()) <
+ std::make_tuple(R.line(), R.column());
+}
+inline bool operator==(const SymbolLocation &L, const SymbolLocation &R) {
+  assert(L.FileURI && R.FileURI);
+  return !std::strcmp(L.FileURI

[PATCH] D58769: Moved DenseMap support for SymbolID into SymbolID.h

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr closed this revision.
gribozavr added a comment.

Committed as https://reviews.llvm.org/rL355081.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58769



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

[PATCH] D58773: Moved SymbolOrigin into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
gribozavr added a reviewer: ioeric.
Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous, 
mgorny.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58773

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/IndexAction.cpp
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/index/SymbolOrigin.cpp
  clang-tools-extra/clangd/index/SymbolOrigin.h
  clang-tools-extra/clangd/index/YAMLSerialization.cpp

Index: clang-tools-extra/clangd/index/YAMLSerialization.cpp
===
--- clang-tools-extra/clangd/index/YAMLSerialization.cpp
+++ clang-tools-extra/clangd/index/YAMLSerialization.cpp
@@ -15,6 +15,7 @@
 #include "Index.h"
 #include "Serialization.h"
 #include "SymbolLocation.h"
+#include "SymbolOrigin.h"
 #include "Trace.h"
 #include "dex/Dex.h"
 #include "llvm/ADT/Optional.h"
Index: clang-tools-extra/clangd/index/SymbolOrigin.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/SymbolOrigin.h
@@ -0,0 +1,47 @@
+//===--- SymbolOrigin.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_INDEX_SYMBOL_ORIGIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_ORIGIN_H
+
+#include "llvm/Support/raw_ostream.h"
+#include 
+
+namespace clang {
+namespace clangd {
+
+// Describes the source of information about a symbol.
+// Mainly useful for debugging, e.g. understanding code completion reuslts.
+// This is a bitfield as information can be combined from several sources.
+enum class SymbolOrigin : uint8_t {
+  Unknown = 0,
+  AST = 1 << 0, // Directly from the AST (indexes should not set this).
+  Dynamic = 1 << 1, // From the dynamic index of opened files.
+  Static = 1 << 2,  // From the static, externally-built index.
+  Merge = 1 << 3,   // A non-trivial index merge was performed.
+  // Remaining bits reserved for index implementations.
+};
+
+inline SymbolOrigin operator|(SymbolOrigin A, SymbolOrigin B) {
+  return static_cast(static_cast(A) |
+   static_cast(B));
+}
+inline SymbolOrigin &operator|=(SymbolOrigin &A, SymbolOrigin B) {
+  return A = A | B;
+}
+inline SymbolOrigin operator&(SymbolOrigin A, SymbolOrigin B) {
+  return static_cast(static_cast(A) &
+   static_cast(B));
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, SymbolOrigin);
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_ORIGIN_H
Index: clang-tools-extra/clangd/index/SymbolOrigin.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/index/SymbolOrigin.cpp
@@ -0,0 +1,25 @@
+//===--- SymbolOrigin.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 "SymbolOrigin.h"
+
+namespace clang {
+namespace clangd {
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, SymbolOrigin O) {
+  if (O == SymbolOrigin::Unknown)
+return OS << "unknown";
+  constexpr static char Sigils[] = "ADSM4567";
+  for (unsigned I = 0; I < sizeof(Sigils); ++I)
+if (static_cast(O) & 1u << I)
+  OS << Sigils[I];
+  return OS;
+}
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -10,6 +10,7 @@
 
 #include "CanonicalIncludes.h"
 #include "Index.h"
+#include "SymbolOrigin.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/Basic/SourceLocation.h"
@@ -143,4 +144,5 @@
 
 } // namespace clangd
 } // namespace clang
+
 #endif
Index: clang-tools-extra/clangd/index/Serialization.cpp
===
--- clang-tools-extra/clangd/index/Serialization.cpp
+++ clang-tools-extra/clangd/index/Serializati

[PATCH] D58774: Moved Symbol into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
gribozavr added a reviewer: ioeric.
Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous, 
mgorny.
Herald added a project: clang.
gribozavr added a parent revision: D58773: Moved SymbolOrigin into its own 
header and implementation file.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58774

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeFixer.cpp
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Symbol.cpp
  clang-tools-extra/clangd/index/Symbol.h

Index: clang-tools-extra/clangd/index/Symbol.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Symbol.h
@@ -0,0 +1,172 @@
+//===--- Symbol.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_INDEX_SYMBOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "SymbolOrigin.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace clangd {
+
+/// The class presents a C++ symbol, e.g. class, function.
+///
+/// WARNING: Symbols do not own much of their underlying data - typically
+/// strings are owned by a SymbolSlab. They should be treated as non-owning
+/// references. Copies are shallow.
+///
+/// When adding new unowned data fields to Symbol, remember to update:
+///   - SymbolSlab::Builder in Index.cpp, to copy them to the slab's storage.
+///   - mergeSymbol in Merge.cpp, to properly combine two Symbols.
+///
+/// A fully documented symbol can be split as:
+/// size_type std::map::count(const K& key) const
+/// | Return  | Scope |Name|Signature |
+/// We split up these components to allow display flexibility later.
+struct Symbol {
+  /// The ID of the symbol.
+  SymbolID ID;
+  /// The symbol information, like symbol kind.
+  index::SymbolInfo SymInfo;
+  /// The unqualified name of the symbol, e.g. "bar" (for ns::bar).
+  llvm::StringRef Name;
+  /// The containing namespace. e.g. "" (global), "ns::" (top-level namespace).
+  llvm::StringRef Scope;
+  /// The location of the symbol's definition, if one was found.
+  /// This just covers the symbol name (e.g. without class/function body).
+  SymbolLocation Definition;
+  /// The location of the preferred declaration of the symbol.
+  /// This just covers the symbol name.
+  /// This may be the same as Definition.
+  ///
+  /// A C++ symbol may have multiple declarations, and we pick one to prefer.
+  ///   * For classes, the canonical declaration should be the definition.
+  ///   * For non-inline functions, the canonical declaration typically appears
+  /// in the ".h" file corresponding to the definition.
+  SymbolLocation CanonicalDeclaration;
+  /// The number of translation units that reference this symbol from their main
+  /// file. This number is only meaningful if aggregated in an index.
+  unsigned References = 0;
+  /// Where this symbol came from. Usually an index provides a constant value.
+  SymbolOrigin Origin = SymbolOrigin::Unknown;
+  /// A brief description of the symbol that can be appended in the completion
+  /// candidate list. For example, "(X x, Y y) const" is a function signature.
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef Signature;
+  /// What to insert when completing this symbol, after the symbol name.
+  /// This is in LSP snippet syntax (e.g. "({$0})" for a no-args function).
+  /// (When snippets are disabled, the symbol name alone is used).
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef CompletionSnippetSuffix;
+  /// Documentation including comment for the symbol declaration.
+  llvm::StringRef Documentation;
+  /// Type when this symbol is used in an expression. (Short display form).
+  /// e.g. return type of a function, or type of a variable.
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef ReturnType;
+
+  /// Raw representation of the OpaqueType of the symbol, used for scoring
+  /// purposes.
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef Type;
+
+  struct IncludeHeaderWithReferences {
+IncludeHeaderWithReferences() = default;
+
+IncludeHeaderWithReferences(llvm::StringRef IncludeHeader,
+unsigned References)
+ 

[PATCH] D58773: Moved SymbolOrigin into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

SymbolOrigin is used by itself, for example, in CodeComplete.h to define 
`struct CodeCompletion`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58773



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

[PATCH] D58773: Moved SymbolOrigin into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE355086: Moved SymbolOrigin into its own header and 
implementation file (authored by gribozavr, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58773?vs=188703&id=188709#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58773

Files:
  clangd/CMakeLists.txt
  clangd/CodeComplete.h
  clangd/index/FileIndex.cpp
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/IndexAction.cpp
  clangd/index/Merge.cpp
  clangd/index/Serialization.cpp
  clangd/index/SymbolCollector.h
  clangd/index/SymbolOrigin.cpp
  clangd/index/SymbolOrigin.h
  clangd/index/YAMLSerialization.cpp

Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -21,6 +21,7 @@
 #include "Path.h"
 #include "Protocol.h"
 #include "index/Index.h"
+#include "index/SymbolOrigin.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/CodeCompleteOptions.h"
Index: clangd/index/SymbolOrigin.h
===
--- clangd/index/SymbolOrigin.h
+++ clangd/index/SymbolOrigin.h
@@ -0,0 +1,47 @@
+//===--- SymbolOrigin.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_INDEX_SYMBOL_ORIGIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_ORIGIN_H
+
+#include "llvm/Support/raw_ostream.h"
+#include 
+
+namespace clang {
+namespace clangd {
+
+// Describes the source of information about a symbol.
+// Mainly useful for debugging, e.g. understanding code completion reuslts.
+// This is a bitfield as information can be combined from several sources.
+enum class SymbolOrigin : uint8_t {
+  Unknown = 0,
+  AST = 1 << 0, // Directly from the AST (indexes should not set this).
+  Dynamic = 1 << 1, // From the dynamic index of opened files.
+  Static = 1 << 2,  // From the static, externally-built index.
+  Merge = 1 << 3,   // A non-trivial index merge was performed.
+  // Remaining bits reserved for index implementations.
+};
+
+inline SymbolOrigin operator|(SymbolOrigin A, SymbolOrigin B) {
+  return static_cast(static_cast(A) |
+   static_cast(B));
+}
+inline SymbolOrigin &operator|=(SymbolOrigin &A, SymbolOrigin B) {
+  return A = A | B;
+}
+inline SymbolOrigin operator&(SymbolOrigin A, SymbolOrigin B) {
+  return static_cast(static_cast(A) &
+   static_cast(B));
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, SymbolOrigin);
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_ORIGIN_H
Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -10,6 +10,7 @@
 
 #include "CanonicalIncludes.h"
 #include "Index.h"
+#include "SymbolOrigin.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/Basic/SourceLocation.h"
@@ -143,4 +144,5 @@
 
 } // namespace clangd
 } // namespace clang
+
 #endif
Index: clangd/index/SymbolOrigin.cpp
===
--- clangd/index/SymbolOrigin.cpp
+++ clangd/index/SymbolOrigin.cpp
@@ -0,0 +1,25 @@
+//===--- SymbolOrigin.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 "SymbolOrigin.h"
+
+namespace clang {
+namespace clangd {
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, SymbolOrigin O) {
+  if (O == SymbolOrigin::Unknown)
+return OS << "unknown";
+  constexpr static char Sigils[] = "ADSM4567";
+  for (unsigned I = 0; I < sizeof(Sigils); ++I)
+if (static_cast(O) & 1u << I)
+  OS << Sigils[I];
+  return OS;
+}
+
+} // namespace clangd
+} // namespace clang
Index: clangd/index/Serialization.cpp
===
--- clangd/index/Serialization.cpp
+++ clangd/index/Serialization.cpp
@@ -10,6 +10,7 @@
 #include "Logger.h"
 #include "RIFF.h"
 #include "SymbolLocation.h"
+#include "SymbolOrigin.h"
 #include "Trace.h"
 #include "dex/Dex.h"
 #include "llvm/Support/Compression.h"
Index: clangd/inde

[PATCH] D58774: Moved Symbol into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188712.
gribozavr added a comment.

Moving SymbolSlab as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58774

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeFixer.cpp
  clang-tools-extra/clangd/IncludeFixer.h
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Serialization.h
  clang-tools-extra/clangd/index/Symbol.cpp
  clang-tools-extra/clangd/index/Symbol.h

Index: clang-tools-extra/clangd/index/Symbol.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Symbol.h
@@ -0,0 +1,231 @@
+//===--- Symbol.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_INDEX_SYMBOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "SymbolOrigin.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/StringSaver.h"
+
+namespace clang {
+namespace clangd {
+
+/// The class presents a C++ symbol, e.g. class, function.
+///
+/// WARNING: Symbols do not own much of their underlying data - typically
+/// strings are owned by a SymbolSlab. They should be treated as non-owning
+/// references. Copies are shallow.
+///
+/// When adding new unowned data fields to Symbol, remember to update:
+///   - SymbolSlab::Builder in Index.cpp, to copy them to the slab's storage.
+///   - mergeSymbol in Merge.cpp, to properly combine two Symbols.
+///
+/// A fully documented symbol can be split as:
+/// size_type std::map::count(const K& key) const
+/// | Return  | Scope |Name|Signature |
+/// We split up these components to allow display flexibility later.
+struct Symbol {
+  /// The ID of the symbol.
+  SymbolID ID;
+  /// The symbol information, like symbol kind.
+  index::SymbolInfo SymInfo;
+  /// The unqualified name of the symbol, e.g. "bar" (for ns::bar).
+  llvm::StringRef Name;
+  /// The containing namespace. e.g. "" (global), "ns::" (top-level namespace).
+  llvm::StringRef Scope;
+  /// The location of the symbol's definition, if one was found.
+  /// This just covers the symbol name (e.g. without class/function body).
+  SymbolLocation Definition;
+  /// The location of the preferred declaration of the symbol.
+  /// This just covers the symbol name.
+  /// This may be the same as Definition.
+  ///
+  /// A C++ symbol may have multiple declarations, and we pick one to prefer.
+  ///   * For classes, the canonical declaration should be the definition.
+  ///   * For non-inline functions, the canonical declaration typically appears
+  /// in the ".h" file corresponding to the definition.
+  SymbolLocation CanonicalDeclaration;
+  /// The number of translation units that reference this symbol from their main
+  /// file. This number is only meaningful if aggregated in an index.
+  unsigned References = 0;
+  /// Where this symbol came from. Usually an index provides a constant value.
+  SymbolOrigin Origin = SymbolOrigin::Unknown;
+  /// A brief description of the symbol that can be appended in the completion
+  /// candidate list. For example, "(X x, Y y) const" is a function signature.
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef Signature;
+  /// What to insert when completing this symbol, after the symbol name.
+  /// This is in LSP snippet syntax (e.g. "({$0})" for a no-args function).
+  /// (When snippets are disabled, the symbol name alone is used).
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef CompletionSnippetSuffix;
+  /// Documentation including comment for the symbol declaration.
+  llvm::StringRef Documentation;
+  /// Type when this symbol is used in an expression. (Short display form).
+  /// e.g. return type of a function, or type of a variable.
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef ReturnType;
+
+  /// Raw representation of the OpaqueType of the symbol, used for scoring
+  /// purposes.
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef Type;
+
+  struct IncludeHeaderWithReferences {
+IncludeHeaderWithReferences() = default;
+
+IncludeHeaderWithReferences(llvm

[PATCH] D58774: Moved Symbol into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Moved SymbolSlab as well, PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58774



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

[PATCH] D58774: Moved Symbol into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188714.
gribozavr added a comment.
Herald added a subscriber: mgrang.

Also moved the SymbolSlab implementation into Symbol.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58774

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeFixer.cpp
  clang-tools-extra/clangd/IncludeFixer.h
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Serialization.h
  clang-tools-extra/clangd/index/Symbol.cpp
  clang-tools-extra/clangd/index/Symbol.h

Index: clang-tools-extra/clangd/index/Symbol.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Symbol.h
@@ -0,0 +1,231 @@
+//===--- Symbol.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_INDEX_SYMBOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOL_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "SymbolOrigin.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/StringSaver.h"
+
+namespace clang {
+namespace clangd {
+
+/// The class presents a C++ symbol, e.g. class, function.
+///
+/// WARNING: Symbols do not own much of their underlying data - typically
+/// strings are owned by a SymbolSlab. They should be treated as non-owning
+/// references. Copies are shallow.
+///
+/// When adding new unowned data fields to Symbol, remember to update:
+///   - SymbolSlab::Builder in Index.cpp, to copy them to the slab's storage.
+///   - mergeSymbol in Merge.cpp, to properly combine two Symbols.
+///
+/// A fully documented symbol can be split as:
+/// size_type std::map::count(const K& key) const
+/// | Return  | Scope |Name|Signature |
+/// We split up these components to allow display flexibility later.
+struct Symbol {
+  /// The ID of the symbol.
+  SymbolID ID;
+  /// The symbol information, like symbol kind.
+  index::SymbolInfo SymInfo;
+  /// The unqualified name of the symbol, e.g. "bar" (for ns::bar).
+  llvm::StringRef Name;
+  /// The containing namespace. e.g. "" (global), "ns::" (top-level namespace).
+  llvm::StringRef Scope;
+  /// The location of the symbol's definition, if one was found.
+  /// This just covers the symbol name (e.g. without class/function body).
+  SymbolLocation Definition;
+  /// The location of the preferred declaration of the symbol.
+  /// This just covers the symbol name.
+  /// This may be the same as Definition.
+  ///
+  /// A C++ symbol may have multiple declarations, and we pick one to prefer.
+  ///   * For classes, the canonical declaration should be the definition.
+  ///   * For non-inline functions, the canonical declaration typically appears
+  /// in the ".h" file corresponding to the definition.
+  SymbolLocation CanonicalDeclaration;
+  /// The number of translation units that reference this symbol from their main
+  /// file. This number is only meaningful if aggregated in an index.
+  unsigned References = 0;
+  /// Where this symbol came from. Usually an index provides a constant value.
+  SymbolOrigin Origin = SymbolOrigin::Unknown;
+  /// A brief description of the symbol that can be appended in the completion
+  /// candidate list. For example, "(X x, Y y) const" is a function signature.
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef Signature;
+  /// What to insert when completing this symbol, after the symbol name.
+  /// This is in LSP snippet syntax (e.g. "({$0})" for a no-args function).
+  /// (When snippets are disabled, the symbol name alone is used).
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef CompletionSnippetSuffix;
+  /// Documentation including comment for the symbol declaration.
+  llvm::StringRef Documentation;
+  /// Type when this symbol is used in an expression. (Short display form).
+  /// e.g. return type of a function, or type of a variable.
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef ReturnType;
+
+  /// Raw representation of the OpaqueType of the symbol, used for scoring
+  /// purposes.
+  /// Only set when the symbol is indexed for completion.
+  llvm::StringRef Type;
+
+  struct IncludeHeaderWithReferences {
+IncludeHeaderWi

[PATCH] D58774: Moved Symbol into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE355088: Moved Symbol into its own header and 
implementation file (authored by gribozavr, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58774?vs=188714&id=188715#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58774

Files:
  clangd/CMakeLists.txt
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Headers.h
  clangd/IncludeFixer.cpp
  clangd/IncludeFixer.h
  clangd/index/Background.cpp
  clangd/index/FileIndex.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/Merge.cpp
  clangd/index/Serialization.h
  clangd/index/Symbol.cpp
  clangd/index/Symbol.h

Index: clangd/CMakeLists.txt
===
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -63,6 +63,7 @@
   index/MemIndex.cpp
   index/Merge.cpp
   index/Serialization.cpp
+  index/Symbol.cpp
   index/SymbolCollector.cpp
   index/SymbolID.cpp
   index/SymbolLocation.cpp
Index: clangd/IncludeFixer.h
===
--- clangd/IncludeFixer.h
+++ clangd/IncludeFixer.h
@@ -12,6 +12,7 @@
 #include "Diagnostics.h"
 #include "Headers.h"
 #include "index/Index.h"
+#include "index/Symbol.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceLocation.h"
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -16,67 +16,6 @@
 namespace clang {
 namespace clangd {
 
-llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, Symbol::SymbolFlag F) {
-  if (F == Symbol::None)
-return OS << "None";
-  std::string S;
-  if (F & Symbol::Deprecated)
-S += "deprecated|";
-  if (F & Symbol::IndexedForCodeCompletion)
-S += "completion|";
-  return OS << llvm::StringRef(S).rtrim('|');
-}
-
-llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Symbol &S) {
-  return OS << S.Scope << S.Name;
-}
-
-float quality(const Symbol &S) {
-  // This avoids a sharp gradient for tail symbols, and also neatly avoids the
-  // question of whether 0 references means a bad symbol or missing data.
-  if (S.References < 3)
-return 1;
-  return std::log(S.References);
-}
-
-SymbolSlab::const_iterator SymbolSlab::find(const SymbolID &ID) const {
-  auto It = std::lower_bound(
-  Symbols.begin(), Symbols.end(), ID,
-  [](const Symbol &S, const SymbolID &I) { return S.ID < I; });
-  if (It != Symbols.end() && It->ID == ID)
-return It;
-  return Symbols.end();
-}
-
-// Copy the underlying data of the symbol into the owned arena.
-static void own(Symbol &S, llvm::UniqueStringSaver &Strings) {
-  visitStrings(S, [&](llvm::StringRef &V) { V = Strings.save(V); });
-}
-
-void SymbolSlab::Builder::insert(const Symbol &S) {
-  auto R = SymbolIndex.try_emplace(S.ID, Symbols.size());
-  if (R.second) {
-Symbols.push_back(S);
-own(Symbols.back(), UniqueStrings);
-  } else {
-auto &Copy = Symbols[R.first->second] = S;
-own(Copy, UniqueStrings);
-  }
-}
-
-SymbolSlab SymbolSlab::Builder::build() && {
-  Symbols = {Symbols.begin(), Symbols.end()}; // Force shrink-to-fit.
-  // Sort symbols so the slab can binary search over them.
-  llvm::sort(Symbols,
- [](const Symbol &L, const Symbol &R) { return L.ID < R.ID; });
-  // We may have unused strings from overwritten symbols. Build a new arena.
-  llvm::BumpPtrAllocator NewArena;
-  llvm::UniqueStringSaver Strings(NewArena);
-  for (auto &S : Symbols)
-own(S, Strings);
-  return SymbolSlab(std::move(NewArena), std::move(Symbols));
-}
-
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, RefKind K) {
   if (K == RefKind::Unknown)
 return OS << "Unknown";
Index: clangd/index/Symbol.cpp
===
--- clangd/index/Symbol.cpp
+++ clangd/index/Symbol.cpp
@@ -0,0 +1,76 @@
+//===--- Symbol.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 "Symbol.h"
+
+namespace clang {
+namespace clangd {
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, Symbol::SymbolFlag F) {
+  if (F == Symbol::None)
+return OS << "None";
+  std::string S;
+  if (F & Symbol::Deprecated)
+S += "deprecated|";
+  if (F & Symbol::IndexedForCodeCompletion)
+S += "completion|";
+  return OS << llvm::StringRef(S).rtrim('|');
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Symbol &S) {
+  return OS << S.Scope << S.Name;
+}
+
+float quality(const Symbol &S) {
+  // Thi

[PATCH] D58778: Moved Ref into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
gribozavr added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous, 
mgorny.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58778

Files:
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/IncludeFixer.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Quality.cpp
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Ref.cpp
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp

Index: clang-tools-extra/clangd/indexer/IndexerMain.cpp
===
--- clang-tools-extra/clangd/indexer/IndexerMain.cpp
+++ clang-tools-extra/clangd/indexer/IndexerMain.cpp
@@ -10,10 +10,10 @@
 //
 //===--===//
 
-#include "index/Index.h"
 #include "index/IndexAction.h"
 #include "index/Merge.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/SymbolCollector.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Execution.h"
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -11,6 +11,7 @@
 #include "CanonicalIncludes.h"
 #include "CodeComplete.h"
 #include "CodeCompletionStrings.h"
+#include "ExpectedTypes.h"
 #include "Logger.h"
 #include "SourceCode.h"
 #include "SymbolLocation.h"
Index: clang-tools-extra/clangd/index/Ref.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Ref.h
@@ -0,0 +1,119 @@
+//===--- Ref.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_INDEX_REF_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REF_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// Describes the kind of a cross-reference.
+///
+/// This is a bitfield which can be combined from different kinds.
+enum class RefKind : uint8_t {
+  Unknown = 0,
+  Declaration = static_cast(index::SymbolRole::Declaration),
+  Definition = static_cast(index::SymbolRole::Definition),
+  Reference = static_cast(index::SymbolRole::Reference),
+  All = Declaration | Definition | Reference,
+};
+
+inline RefKind operator|(RefKind L, RefKind R) {
+  return static_cast(static_cast(L) |
+  static_cast(R));
+}
+inline RefKind &operator|=(RefKind &L, RefKind R) { return L = L | R; }
+inline RefKind operator&(RefKind A, RefKind B) {
+  return static_cast(static_cast(A) &
+  static_cast(B));
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, RefKind);
+
+/// Represents a symbol occurrence in the source file.
+/// Despite the name, it could be a declaration/definition/reference.
+///
+/// WARNING: Location does not own the underlying data - Copies are shallow.
+struct Ref {
+  /// The source location where the symbol is named.
+  SymbolLocation Location;
+  RefKind Kind = RefKind::Unknown;
+};
+
+inline bool operator<(const Ref &L, const Ref &R) {
+  return std::tie(L.Location, L.Kind) < std::tie(R.Location, R.Kind);
+}
+inline bool operator==(const Ref &L, const Ref &R) {
+  return std::tie(L.Location, L.Kind) == std::tie(R.Location, R.Kind);
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Ref &);
+
+/// An efficient structure of storing large set of symbol references in memory.
+/// Filenames are deduplicated.
+class RefSlab {
+public:
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;
+  using iterator = const_iterator;
+
+  RefSlab() = default;
+  RefSlab(RefSlab &&Slab) = default;
+  RefSlab &operator=(RefSlab &&RHS) = default;
+
+  const_iterator begin() const { return Refs.begin(); }
+  const_iterator end() const { return Refs.end(); }
+  /// Gets the number of symbols.
+  size_t size() const { return Refs.size(); }
+  size_t numRefs() const { return NumRefs; }
+  bool empty() const { return Refs.empty(); }
+
+  size_t bytes() const {
+return sizeof(*this) + Arena.getTotalMemory() +
+   sizeof(value_type) * Refs.si

[PATCH] D58778: Moved Ref into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188723.
gribozavr marked an inline comment as done.
gribozavr added a comment.

Added a license header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58778

Files:
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/IncludeFixer.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Quality.cpp
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Ref.cpp
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp

Index: clang-tools-extra/clangd/indexer/IndexerMain.cpp
===
--- clang-tools-extra/clangd/indexer/IndexerMain.cpp
+++ clang-tools-extra/clangd/indexer/IndexerMain.cpp
@@ -10,10 +10,10 @@
 //
 //===--===//
 
-#include "index/Index.h"
 #include "index/IndexAction.h"
 #include "index/Merge.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/SymbolCollector.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Execution.h"
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -11,6 +11,7 @@
 #include "CanonicalIncludes.h"
 #include "CodeComplete.h"
 #include "CodeCompletionStrings.h"
+#include "ExpectedTypes.h"
 #include "Logger.h"
 #include "SourceCode.h"
 #include "SymbolLocation.h"
Index: clang-tools-extra/clangd/index/Ref.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Ref.h
@@ -0,0 +1,119 @@
+//===--- Ref.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_INDEX_REF_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REF_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// Describes the kind of a cross-reference.
+///
+/// This is a bitfield which can be combined from different kinds.
+enum class RefKind : uint8_t {
+  Unknown = 0,
+  Declaration = static_cast(index::SymbolRole::Declaration),
+  Definition = static_cast(index::SymbolRole::Definition),
+  Reference = static_cast(index::SymbolRole::Reference),
+  All = Declaration | Definition | Reference,
+};
+
+inline RefKind operator|(RefKind L, RefKind R) {
+  return static_cast(static_cast(L) |
+  static_cast(R));
+}
+inline RefKind &operator|=(RefKind &L, RefKind R) { return L = L | R; }
+inline RefKind operator&(RefKind A, RefKind B) {
+  return static_cast(static_cast(A) &
+  static_cast(B));
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, RefKind);
+
+/// Represents a symbol occurrence in the source file.
+/// Despite the name, it could be a declaration/definition/reference.
+///
+/// WARNING: Location does not own the underlying data - Copies are shallow.
+struct Ref {
+  /// The source location where the symbol is named.
+  SymbolLocation Location;
+  RefKind Kind = RefKind::Unknown;
+};
+
+inline bool operator<(const Ref &L, const Ref &R) {
+  return std::tie(L.Location, L.Kind) < std::tie(R.Location, R.Kind);
+}
+inline bool operator==(const Ref &L, const Ref &R) {
+  return std::tie(L.Location, L.Kind) == std::tie(R.Location, R.Kind);
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Ref &);
+
+/// An efficient structure of storing large set of symbol references in memory.
+/// Filenames are deduplicated.
+class RefSlab {
+public:
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;
+  using iterator = const_iterator;
+
+  RefSlab() = default;
+  RefSlab(RefSlab &&Slab) = default;
+  RefSlab &operator=(RefSlab &&RHS) = default;
+
+  const_iterator begin() const { return Refs.begin(); }
+  const_iterator end() const { return Refs.end(); }
+  /// Gets the number of symbols.
+  size_t size() const { return Refs.size(); }
+  size_t numRefs() const { return NumRefs; }
+  bool empty() const { return Refs.empty(); }
+
+  size_t bytes() const {
+return sizeof(*this) + Arena.getTotalMemory() +
+   si

[PATCH] D58778: Moved Ref into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr updated this revision to Diff 188724.
gribozavr marked 2 inline comments as done.
gribozavr added a comment.

Added a missing include.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58778

Files:
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/IncludeFixer.cpp
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Quality.cpp
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/Ref.cpp
  clang-tools-extra/clangd/index/Ref.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/indexer/IndexerMain.cpp

Index: clang-tools-extra/clangd/indexer/IndexerMain.cpp
===
--- clang-tools-extra/clangd/indexer/IndexerMain.cpp
+++ clang-tools-extra/clangd/indexer/IndexerMain.cpp
@@ -10,10 +10,11 @@
 //
 //===--===//
 
-#include "index/Index.h"
 #include "index/IndexAction.h"
 #include "index/Merge.h"
+#include "index/Ref.h"
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/SymbolCollector.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Execution.h"
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -11,6 +11,7 @@
 #include "CanonicalIncludes.h"
 #include "CodeComplete.h"
 #include "CodeCompletionStrings.h"
+#include "ExpectedTypes.h"
 #include "Logger.h"
 #include "SourceCode.h"
 #include "SymbolLocation.h"
Index: clang-tools-extra/clangd/index/Ref.h
===
--- /dev/null
+++ clang-tools-extra/clangd/index/Ref.h
@@ -0,0 +1,119 @@
+//===--- Ref.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_INDEX_REF_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REF_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// Describes the kind of a cross-reference.
+///
+/// This is a bitfield which can be combined from different kinds.
+enum class RefKind : uint8_t {
+  Unknown = 0,
+  Declaration = static_cast(index::SymbolRole::Declaration),
+  Definition = static_cast(index::SymbolRole::Definition),
+  Reference = static_cast(index::SymbolRole::Reference),
+  All = Declaration | Definition | Reference,
+};
+
+inline RefKind operator|(RefKind L, RefKind R) {
+  return static_cast(static_cast(L) |
+  static_cast(R));
+}
+inline RefKind &operator|=(RefKind &L, RefKind R) { return L = L | R; }
+inline RefKind operator&(RefKind A, RefKind B) {
+  return static_cast(static_cast(A) &
+  static_cast(B));
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, RefKind);
+
+/// Represents a symbol occurrence in the source file.
+/// Despite the name, it could be a declaration/definition/reference.
+///
+/// WARNING: Location does not own the underlying data - Copies are shallow.
+struct Ref {
+  /// The source location where the symbol is named.
+  SymbolLocation Location;
+  RefKind Kind = RefKind::Unknown;
+};
+
+inline bool operator<(const Ref &L, const Ref &R) {
+  return std::tie(L.Location, L.Kind) < std::tie(R.Location, R.Kind);
+}
+inline bool operator==(const Ref &L, const Ref &R) {
+  return std::tie(L.Location, L.Kind) == std::tie(R.Location, R.Kind);
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Ref &);
+
+/// An efficient structure of storing large set of symbol references in memory.
+/// Filenames are deduplicated.
+class RefSlab {
+public:
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;
+  using iterator = const_iterator;
+
+  RefSlab() = default;
+  RefSlab(RefSlab &&Slab) = default;
+  RefSlab &operator=(RefSlab &&RHS) = default;
+
+  const_iterator begin() const { return Refs.begin(); }
+  const_iterator end() const { return Refs.end(); }
+  /// Gets the number of symbols.
+  size_t size() const { return Refs.size(); }
+  size_t numRefs() const { return NumRefs; }
+  bool empty() const { return Refs.empty(); }
+
+  size_t bytes() const {
+return sizeof(*this) + Arena.getTotal

[PATCH] D58778: Moved Ref into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/index/Ref.cpp:1
+#include "Ref.h"
+

ioeric wrote:
> License?
Added.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:14
 #include "CodeCompletionStrings.h"
+#include "ExpectedTypes.h"
 #include "Logger.h"

ioeric wrote:
> Is this change relevant in this patch?
Yes, once I replaced inclusion of Index.h with a more specific file somewhere 
else, SymbolCollector.cpp stopped building because it relied on transitive 
inclusion of ExpectedTypes.h.



Comment at: clang-tools-extra/clangd/indexer/IndexerMain.cpp:16
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/SymbolCollector.h"

ioeric wrote:
> Maybe also include Ref.h?
Added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58778



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

[PATCH] D58781: Added missing license headers

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
gribozavr added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58781

Files:
  clang-tools-extra/clangd/ExpectedTypes.cpp
  clang-tools-extra/clangd/index/IndexAction.cpp
  clang-tools-extra/clangd/xpc/framework/ClangdXPC.cpp
  clang-tools-extra/clangd/xpc/test-client/ClangdXPCTestClient.cpp


Index: clang-tools-extra/clangd/xpc/test-client/ClangdXPCTestClient.cpp
===
--- clang-tools-extra/clangd/xpc/test-client/ClangdXPCTestClient.cpp
+++ clang-tools-extra/clangd/xpc/test-client/ClangdXPCTestClient.cpp
@@ -1,3 +1,11 @@
+//===--- ClangXPCTestClient.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 "xpc/Conversion.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallString.h"
Index: clang-tools-extra/clangd/xpc/framework/ClangdXPC.cpp
===
--- clang-tools-extra/clangd/xpc/framework/ClangdXPC.cpp
+++ clang-tools-extra/clangd/xpc/framework/ClangdXPC.cpp
@@ -1,3 +1,10 @@
+//===--- ClangXPC.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
+//
+//===--===//
 
 /// Returns the bundle identifier of the Clangd XPC service.
 extern "C" const char *clangd_xpc_get_bundle_identifier() {
Index: clang-tools-extra/clangd/index/IndexAction.cpp
===
--- clang-tools-extra/clangd/index/IndexAction.cpp
+++ clang-tools-extra/clangd/index/IndexAction.cpp
@@ -1,5 +1,12 @@
-#include "IndexAction.h"
+//===--- IndexAction.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 "IndexAction.h"
 #include "index/SymbolOrigin.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexDataConsumer.h"
Index: clang-tools-extra/clangd/ExpectedTypes.cpp
===
--- clang-tools-extra/clangd/ExpectedTypes.cpp
+++ clang-tools-extra/clangd/ExpectedTypes.cpp
@@ -1,3 +1,11 @@
+//===--- ExpectedTypes.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 "ExpectedTypes.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Type.h"


Index: clang-tools-extra/clangd/xpc/test-client/ClangdXPCTestClient.cpp
===
--- clang-tools-extra/clangd/xpc/test-client/ClangdXPCTestClient.cpp
+++ clang-tools-extra/clangd/xpc/test-client/ClangdXPCTestClient.cpp
@@ -1,3 +1,11 @@
+//===--- ClangXPCTestClient.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 "xpc/Conversion.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallString.h"
Index: clang-tools-extra/clangd/xpc/framework/ClangdXPC.cpp
===
--- clang-tools-extra/clangd/xpc/framework/ClangdXPC.cpp
+++ clang-tools-extra/clangd/xpc/framework/ClangdXPC.cpp
@@ -1,3 +1,10 @@
+//===--- ClangXPC.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
+//
+//===--===//
 
 /// Returns the bundle identifier of the Clangd XPC service.
 extern "C" const char *clangd_xpc_

[PATCH] D58778: Moved Ref into its own header and implementation file

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
gribozavr marked an inline comment as done.
Closed by commit rL355090: Moved Ref into its own header and implementation 
file (authored by gribozavr, committed by ).
Herald added subscribers: llvm-commits, ilya-biryukov.
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D58778?vs=188724&id=188727#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58778

Files:
  clang-tools-extra/trunk/clangd/AST.h
  clang-tools-extra/trunk/clangd/CMakeLists.txt
  clang-tools-extra/trunk/clangd/IncludeFixer.cpp
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Quality.cpp
  clang-tools-extra/trunk/clangd/index/Index.cpp
  clang-tools-extra/trunk/clangd/index/Index.h
  clang-tools-extra/trunk/clangd/index/Ref.cpp
  clang-tools-extra/trunk/clangd/index/Ref.h
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/indexer/IndexerMain.cpp

Index: clang-tools-extra/trunk/clangd/CMakeLists.txt
===
--- clang-tools-extra/trunk/clangd/CMakeLists.txt
+++ clang-tools-extra/trunk/clangd/CMakeLists.txt
@@ -62,6 +62,7 @@
   index/IndexAction.cpp
   index/MemIndex.cpp
   index/Merge.cpp
+  index/Ref.cpp
   index/Serialization.cpp
   index/Symbol.cpp
   index/SymbolCollector.cpp
Index: clang-tools-extra/trunk/clangd/index/Index.cpp
===
--- clang-tools-extra/trunk/clangd/index/Index.cpp
+++ clang-tools-extra/trunk/clangd/index/Index.cpp
@@ -12,57 +12,11 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
 
 namespace clang {
 namespace clangd {
 
-llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, RefKind K) {
-  if (K == RefKind::Unknown)
-return OS << "Unknown";
-  static const std::vector Messages = {"Decl", "Def", "Ref"};
-  bool VisitedOnce = false;
-  for (unsigned I = 0; I < Messages.size(); ++I) {
-if (static_cast(K) & 1u << I) {
-  if (VisitedOnce)
-OS << ", ";
-  OS << Messages[I];
-  VisitedOnce = true;
-}
-  }
-  return OS;
-}
-
-llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Ref &R) {
-  return OS << R.Location << ":" << R.Kind;
-}
-
-void RefSlab::Builder::insert(const SymbolID &ID, const Ref &S) {
-  auto &M = Refs[ID];
-  M.push_back(S);
-  M.back().Location.FileURI =
-  UniqueStrings.save(M.back().Location.FileURI).data();
-}
-
-RefSlab RefSlab::Builder::build() && {
-  // We can reuse the arena, as it only has unique strings and we need them all.
-  // Reallocate refs on the arena to reduce waste and indirections when reading.
-  std::vector>> Result;
-  Result.reserve(Refs.size());
-  size_t NumRefs = 0;
-  for (auto &Sym : Refs) {
-auto &SymRefs = Sym.second;
-llvm::sort(SymRefs);
-// FIXME: do we really need to dedup?
-SymRefs.erase(std::unique(SymRefs.begin(), SymRefs.end()), SymRefs.end());
-
-NumRefs += SymRefs.size();
-auto *Array = Arena.Allocate(SymRefs.size());
-std::uninitialized_copy(SymRefs.begin(), SymRefs.end(), Array);
-Result.emplace_back(Sym.first, llvm::ArrayRef(Array, SymRefs.size()));
-  }
-  return RefSlab(std::move(Result), std::move(Arena), NumRefs);
-}
-
 void SwapIndex::reset(std::unique_ptr Index) {
   // Keep the old index alive, so we don't destroy it under lock (may be slow).
   std::shared_ptr Pin;
Index: clang-tools-extra/trunk/clangd/index/Ref.h
===
--- clang-tools-extra/trunk/clangd/index/Ref.h
+++ clang-tools-extra/trunk/clangd/index/Ref.h
@@ -0,0 +1,119 @@
+//===--- Ref.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_INDEX_REF_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REF_H
+
+#include "SymbolID.h"
+#include "SymbolLocation.h"
+#include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/StringSaver.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// Describes the kind of a cross-reference.
+///
+/// This is a bitfield which can be combined from different kinds.
+enum class RefKind : uint8_t {
+  Unknown = 0,
+  Declaration = static_cast(index::SymbolRole::Declaration),
+  Definition = static_cast(index::SymbolRole::Definition),
+  Reference = static_cast(index::SymbolRole::Reference),
+  All = Declaration | Definition | Reference,
+};
+
+inline RefKind operator|(RefKind L

[PATCH] D58782: Use ArrayRef::copy, instead of copying data manually

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
gribozavr added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58782

Files:
  clang-tools-extra/clangd/index/Ref.cpp


Index: clang-tools-extra/clangd/index/Ref.cpp
===
--- clang-tools-extra/clangd/index/Ref.cpp
+++ clang-tools-extra/clangd/index/Ref.cpp
@@ -51,9 +51,7 @@
 SymRefs.erase(std::unique(SymRefs.begin(), SymRefs.end()), SymRefs.end());
 
 NumRefs += SymRefs.size();
-auto *Array = Arena.Allocate(SymRefs.size());
-std::uninitialized_copy(SymRefs.begin(), SymRefs.end(), Array);
-Result.emplace_back(Sym.first, llvm::ArrayRef(Array, SymRefs.size()));
+Result.emplace_back(Sym.first, llvm::ArrayRef(SymRefs).copy(Arena));
   }
   return RefSlab(std::move(Result), std::move(Arena), NumRefs);
 }


Index: clang-tools-extra/clangd/index/Ref.cpp
===
--- clang-tools-extra/clangd/index/Ref.cpp
+++ clang-tools-extra/clangd/index/Ref.cpp
@@ -51,9 +51,7 @@
 SymRefs.erase(std::unique(SymRefs.begin(), SymRefs.end()), SymRefs.end());
 
 NumRefs += SymRefs.size();
-auto *Array = Arena.Allocate(SymRefs.size());
-std::uninitialized_copy(SymRefs.begin(), SymRefs.end(), Array);
-Result.emplace_back(Sym.first, llvm::ArrayRef(Array, SymRefs.size()));
+Result.emplace_back(Sym.first, llvm::ArrayRef(SymRefs).copy(Arena));
   }
   return RefSlab(std::move(Result), std::move(Arena), NumRefs);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58782: Use ArrayRef::copy, instead of copying data manually

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE355091: Use ArrayRef::copy, instead of copying data 
manually (authored by gribozavr, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58782?vs=188729&id=188730#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58782

Files:
  clangd/index/Ref.cpp


Index: clangd/index/Ref.cpp
===
--- clangd/index/Ref.cpp
+++ clangd/index/Ref.cpp
@@ -51,9 +51,7 @@
 SymRefs.erase(std::unique(SymRefs.begin(), SymRefs.end()), SymRefs.end());
 
 NumRefs += SymRefs.size();
-auto *Array = Arena.Allocate(SymRefs.size());
-std::uninitialized_copy(SymRefs.begin(), SymRefs.end(), Array);
-Result.emplace_back(Sym.first, llvm::ArrayRef(Array, SymRefs.size()));
+Result.emplace_back(Sym.first, llvm::ArrayRef(SymRefs).copy(Arena));
   }
   return RefSlab(std::move(Result), std::move(Arena), NumRefs);
 }


Index: clangd/index/Ref.cpp
===
--- clangd/index/Ref.cpp
+++ clangd/index/Ref.cpp
@@ -51,9 +51,7 @@
 SymRefs.erase(std::unique(SymRefs.begin(), SymRefs.end()), SymRefs.end());
 
 NumRefs += SymRefs.size();
-auto *Array = Arena.Allocate(SymRefs.size());
-std::uninitialized_copy(SymRefs.begin(), SymRefs.end(), Array);
-Result.emplace_back(Sym.first, llvm::ArrayRef(Array, SymRefs.size()));
+Result.emplace_back(Sym.first, llvm::ArrayRef(SymRefs).copy(Arena));
   }
   return RefSlab(std::move(Result), std::move(Arena), NumRefs);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58781: Added missing license headers

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE355092: Added missing license headers (authored by 
gribozavr, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58781?vs=188726&id=188731#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58781

Files:
  clangd/ExpectedTypes.cpp
  clangd/index/IndexAction.cpp
  clangd/xpc/framework/ClangdXPC.cpp
  clangd/xpc/test-client/ClangdXPCTestClient.cpp


Index: clangd/ExpectedTypes.cpp
===
--- clangd/ExpectedTypes.cpp
+++ clangd/ExpectedTypes.cpp
@@ -1,3 +1,11 @@
+//===--- ExpectedTypes.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 "ExpectedTypes.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Type.h"
Index: clangd/index/IndexAction.cpp
===
--- clangd/index/IndexAction.cpp
+++ clangd/index/IndexAction.cpp
@@ -1,5 +1,12 @@
-#include "IndexAction.h"
+//===--- IndexAction.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 "IndexAction.h"
 #include "index/SymbolOrigin.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexDataConsumer.h"
Index: clangd/xpc/framework/ClangdXPC.cpp
===
--- clangd/xpc/framework/ClangdXPC.cpp
+++ clangd/xpc/framework/ClangdXPC.cpp
@@ -1,3 +1,10 @@
+//===--- ClangXPC.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
+//
+//===--===//
 
 /// Returns the bundle identifier of the Clangd XPC service.
 extern "C" const char *clangd_xpc_get_bundle_identifier() {
Index: clangd/xpc/test-client/ClangdXPCTestClient.cpp
===
--- clangd/xpc/test-client/ClangdXPCTestClient.cpp
+++ clangd/xpc/test-client/ClangdXPCTestClient.cpp
@@ -1,3 +1,11 @@
+//===--- ClangXPCTestClient.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 "xpc/Conversion.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallString.h"


Index: clangd/ExpectedTypes.cpp
===
--- clangd/ExpectedTypes.cpp
+++ clangd/ExpectedTypes.cpp
@@ -1,3 +1,11 @@
+//===--- ExpectedTypes.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 "ExpectedTypes.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Type.h"
Index: clangd/index/IndexAction.cpp
===
--- clangd/index/IndexAction.cpp
+++ clangd/index/IndexAction.cpp
@@ -1,5 +1,12 @@
-#include "IndexAction.h"
+//===--- IndexAction.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 "IndexAction.h"
 #include "index/SymbolOrigin.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexDataConsumer.h"
Index: clangd/xpc/framework/ClangdXPC.cpp
===
--- clangd/xpc/framework/ClangdXPC.cpp
+++ clangd/xpc/framework/ClangdXPC.cpp
@@ -1,3 +1,10 @@
+//===--- ClangXPC.cpp --

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Dmitri Gribenko via Phabricator via cfe-commits]

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

Please also add a test for find references on a constructor.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D58815: [clangd] Make sure constructors do not reference class

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: unittests/clangd/XRefsTests.cpp:1361
+}
+  )cpp",
   };

Add the new test right after the "Method call" test, to keep it grouped?  
Constructor calls are kinda like method calls.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58815



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

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> These references were added to support using the index data for symbol rename.

Understood -- thank you for providing the context (the original commit that 
added this code didn't have the explanation why this reference was added).  
However, my suggestion would be to still take this patch.  For symbol rename, 
my suggestion would be one of the following.

Option (1): Symbol rename is a complex operation that requires knowing many 
places where the class name appears.  So I think it is fair that it would need 
to navigate to all such declarations using the semantic index -- find the 
class, then find its constructors, destructor, out-of-line functions, find 
derived classes, then find `using` declarations in derived classes, find calls 
to constructors, destructor etc.  I don't think it is not the job of the core 
indexer API to hardcode all of these places as a "reference to the class".  
Different clients require different navigation to related symbols, and 
hardcoding all such navigation patterns in the indexer would create a messy 
index that is difficult to work with, since you'd have to filter out lots of 
stuff.  Not to even mention the index size.

Option (2): A client that wants to hardcode all navigation patterns in the 
index can still do this -- in the `IndexDataConsumer`, it is possible to handle 
the `ConstructorDecl` as a reference to the class, if desired.  The flow of the 
indexing information becomes more clear in my opinion: when there is a 
constructor decl in the source, the `IndexDataConsumer` gets one 
`ConstructorDecl`. Then the specific implementation of `IndexDataConsumer` 
decides to treat it also as a class reference, because it wants to support a 
specific approach to performing symbol renaming.

As is, the indexing information is misleading and surprising.  When we see a 
constructor decl or a constructor call, there is no reference to the class at 
that point -- there is a reference to a constructor.  I think index is supposed 
to expose semantic information, not just that there's a token in the source 
code that has the same spelling as the class name.

> could we instead distinguish these cases with a more specific SymbolRole, 
> e.g. NameReference as opposed to Reference, and filter them based on that 
> instead?

It feels like a workaround to me, that also pushes the fix to the clients of 
the indexing API. The core of the problem still exists: the indexing 
information is surprising, and requires extra work on the client to make it not 
surprising (filtering out NameReferences).


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D57112: [ASTTypeTraits] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> The test coverage i have will be in the clang-tidy check.

Please add unit tests to `clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp`. 
 You can pull matchers from D57113  into the 
shared matchers library (instead of keeping them private to the check), and add 
unit tests.




Comment at: include/clang/AST/ASTTypeTraits.h:83
+  /// Return the AST node kind of this ASTNodeKind.
+  OpenMPClauseKind getOMPClauseKind() const;
+  /// \}

"asOMPClauseKind()"?



Comment at: lib/AST/ASTTypeTraits.cpp:114
+#define OPENMP_CLAUSE(Name, Class) 
\
+case OMPC_##Name: return ASTNodeKind(NKI_##Class);
+#include "clang/Basic/OpenMPKinds.def"

lebedev.ri wrote:
> lebedev.ri wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > lebedev.ri wrote:
> > > > > ABataev wrote:
> > > > > > Well, I think it would be good to filter out `OMPC_flush` somehow 
> > > > > > because there is no such clause actually, it is a pseudo clause for 
> > > > > > better handling of the `flush` directive.
> > > > > > 
> > > > > Are `OMPC_threadprivate` and `OMPC_uniform` also in the same boat?
> > > > > I don't see those clauses in spec.
> > > > > 
> > > > > Perhaps `OMPC_flush` should be made more like those other two?
> > > > > (i.e. handled outside of `OPENMP_CLAUSE` macro)
> > > > `OMPC_threadprivate` is also a special kind of pseudo-clause.
> > > > `OMPC_flush` is in the enum, because it has the corresponding class. 
> > > > You can try to exclude it from the enum, but it may require some 
> > > > additional work.
> > > > `OMPC_uniform` is a normal clause, but it has the corresponding class. 
> > > > This clause can be used on `declare simd` directive, which is 
> > > > represented as an attribute.
> > > I mean, `OOMPC_uniform` has no(!) corresponding class. Mistyped
> > As one would expect, simply adding 
> > ```
> >   case OMPC_flush: // Pseudo clause, does not exist (keep before including 
> > .def)
> > llvm_unreachable("unexpected OpenMP clause kind");
> > ```
> > results in a
> > ```
> > [58/1118 5.6/sec] Building CXX object 
> > tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o
> > FAILED: tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o 
> > /usr/bin/g++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE 
> > -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
> > -Itools/clang/lib/AST -I/build/clang/lib/AST -I/build/clang/include 
> > -Itools/clang/include -I/usr/include/libxml2 -Iinclude 
> > -I/build/llvm/include -pipe -O2 -g0 -UNDEBUG -fPIC 
> > -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra 
> > -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
> > -Wno-missing-field-initializers -pedantic -Wno-long-long 
> > -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess 
> > -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment 
> > -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common 
> > -Woverloaded-virtual -fno-strict-aliasing -pipe -O2 -g0 -UNDEBUG -fPIC   
> > -UNDEBUG  -fno-exceptions -fno-rtti -MD -MT 
> > tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o -MF 
> > tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o.d -o 
> > tools/clang/lib/AST/CMakeFiles/clangAST.dir/ASTTypeTraits.cpp.o -c 
> > /build/clang/lib/AST/ASTTypeTraits.cpp
> > /build/clang/include/clang/Basic/OpenMPKinds.def: In static member function 
> > ‘static clang::ast_type_traits::ASTNodeKind 
> > clang::ast_type_traits::ASTNodeKind::getFromNode(const clang::OMPClause&)’:
> > /build/clang/lib/AST/ASTTypeTraits.cpp:116:5: error: duplicate case value
> >  case OMPC_##Name: return ASTNodeKind(NKI_##Class);
> >  ^~~~
> > /build/clang/include/clang/Basic/OpenMPKinds.def:261:1: note: in expansion 
> > of macro ‘OPENMP_CLAUSE’
> >  OPENMP_CLAUSE(flush, OMPFlushClause)
> >  ^
> > /build/clang/lib/AST/ASTTypeTraits.cpp:113:3: note: previously used here
> >case OMPC_flush: // Pseudo clause, does not exist (keep before including 
> > .def)
> >^~~~
> > ```
> > So one will need to pull `OMPC_flush` out of `clang/Basic/OpenMPKinds.def`.
> D57280, will rebase this.
> Well, I think it would be good to filter out `OMPC_flush` somehow because 
> there is no such clause actually, it is a pseudo clause for better handling 
> of the `flush` directive.

Sorry to be late for this discussion, but I don't think this conclusion 
follows.  ASTMatchers are supposed to match the AST as it is.  Even if 
`OMPC_flush` is synthetic, it exists in the AST, and users might want to match 
it.  I think users would find anything else (trying to filter out AST nodes 
that are not in the source code) to be surprising. For example, there's a 
matcher `materializeTemporaryExpr` even though this AST node is a Clang 
invention and is not a part of the C++ spec.

Matching only c

[PATCH] D39050: Add index-while-building support to Clang

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.
Herald added a subscriber: jdoerfert.



Comment at: include/clang/Frontend/FrontendOptions.h:377
 
+  /// The path to write index data to
+  std::string IndexStorePath;

Please end comments with a period.



Comment at: include/clang/Frontend/FrontendOptions.h:380
+  /// Whether to ignore system files when writing out index data
+  unsigned IndexIgnoreSystemSymbols : 1;
+  /// Whether to include the codegen name of symbols in the index data

Would it make more sense to flip this boolean to positive?  
"IndexIncludeSystemSymbols"?



Comment at: lib/Index/IndexingAction.cpp:102
+  /// \param CI The compiler instance used to process the input
+  /// \returns the created IndexASTConsumer.
+  virtual std::unique_ptr

Please don't duplicate the information from the signature in comments.  No need 
to say that this function returns an IndexASTConsumer (twice, in the first 
sentence and in the \returns clause), the code already says that.

Also, "The compiler instance used to process the input" does not mean much to 
me either.



Comment at: lib/Index/IndexingAction.cpp:154
+return llvm::make_unique(std::move(Consumers));
+  };
+

No semicolon.



Comment at: lib/Index/IndexingAction.cpp:163
+}
+  };
 };

No semicolon.



Comment at: lib/Index/IndexingAction.cpp:186
+  void finish(CompilerInstance &CI) override { DataConsumer->finish(); }
+};
+

No semicolon.



Comment at: lib/Index/IndexingAction.cpp:275
+/// Construct a \c UnitDetails from the invocation associated with the provided
+/// \c CompilerInstance and the provided sysroot path.
+static index::UnitDetails getUnitDetails(const CompilerInstance &CI,

Please don't duplicate type information from the signature in the comment.




Comment at: lib/Index/IndexingAction.cpp:283
+OutputFile = CI.getFrontendOpts().Inputs[0].getFile();
+OutputFile += ".o";
+  }

I don't understand... this is not really the user-specified output file.



Comment at: lib/Index/IndexingAction.cpp:303
+
+/// Construct a \c UnitDetails for the given module file.
+static index::UnitDetails getUnitDetails(serialization::ModuleFile &Mod,

Please don't duplicate type information from the signature in the comment.



Comment at: lib/Index/IndexingContext.h:40
+/// Tracks the current system root path and computes and caches whether a
+/// file is considered a system file or not
+class SystemFileCache {

Please add a period at the end of the comment.



Comment at: lib/Index/IndexingContext.h:44
+  // Records whether a directory entry is system or not.
+  llvm::DenseMap DirEntries;
+  // Keeps track of the last check for whether a FileID is system or

DirEntries => IsSystemDirEntry?



Comment at: lib/Index/IndexingContext.h:46
+  // Keeps track of the last check for whether a FileID is system or
+  // not. This is used to speed up isSystemFile() call.
+  std::pair LastFileCheck;

Triple slashes for doc comments.



Comment at: lib/Index/IndexingContext.h:46
+  // Keeps track of the last check for whether a FileID is system or
+  // not. This is used to speed up isSystemFile() call.
+  std::pair LastFileCheck;

gribozavr wrote:
> Triple slashes for doc comments.
Unclear how a boolean can keep track of the last check.

Did you mean "Whether the file is a system file or not.  This value is a 
cache."  If so, please rename the variable to something like IsSystemFileCache.



Comment at: test/Index/Core/index-source.mm:2
 // RUN: c-index-test core -print-source-symbols -- %s -target 
x86_64-apple-macosx10.7 | FileCheck %s
+// RUN: c-index-test core -print-source-unit -- %s -target 
x86_64-apple-macosx10.7 | FileCheck -check-prefixes=CHECK %s
 

No need to specify check-prefixes=CHECK.



Comment at: test/Index/Core/index-unit.mm:1
+// RUN: rm -rf %t.mcp
+// RUN: c-index-test core -print-source-unit -- -arch x86_64 
-mmacosx-version-min=10.7 -c %s -o %t.o -isystem %S/Inputs/sys -fmodules 
-fmodules-cache-path=%t.mcp -Xclang -fdisable-module-hash -I %S/Inputs/module 
-I %S/Inputs | FileCheck %s

This test is very difficult to read... it is just a dump of random internal 
data structures... what do you think about converting it to a unit test?


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

https://reviews.llvm.org/D39050



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

[PATCH] D58979: [clang][OpenMP] Revert "OMPFlushClause is synthetic, no such clause exists"

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added a comment.

> Hmm, do we really need the matches for the AST node that is not 
> described/defined by the standard? If this is really so, I'm ok with this.

When the rubber meets the road, it does not matter what is in the spec, users 
will need to work with the ASTs that they get.  If there's something in the 
AST, users will want to match it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58979



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

[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> My understanding is that the storage space taken up for Refs is currently 8 
> bytes per Ref (4 each for the start and end positions), plus filename strings 
> which are deduplicated across all refs. If we add a SymbolID, that adds an 
> additional 8 bytes to each Ref. Given that Refs are numerous, and most of 
> them won't use the SymbolID, that seems wasteful.

I see.  This is a very good observation, we didn't consider increase in memory 
usage when we discussed your patch.  Sorry about that!  **We discussed your 
patch again and we agree with the approach of adding a RelationsSlab.**

Here are the options we considered, and the trade-offs we identified.

Option 1: Add SymbolID to Ref.  Advantage: simpler API, fewer types in the API. 
 Disadvantage: increased memory usage, as you noted.

Option 2: Add Relation struct, but store them in RefSlab, using a compact 
representation that skips storing SymbolID where it is not present.  
Advantages: we avoid adding a type that's almost an exact duplicate of RefSlab, 
we don't unnecessarily increase memory usage.  Disadvantages: tricky code to 
avoid storing SymbolID, and RefSlab now stores both Refs and Relations, which 
is confusing from the API point of view.

Option 3 (what your patch implements): Add Relation struct and RelationSlab.  
Advantage: API matches the data model, we don't unnecessarily increase memory 
usage.  Disadvantage: have to plumb the new slab everywhere in the code.

Option 4: Add a Relation struct.  Change RefSlab to a template so that it can 
store either Refs, or Relations.  Advantage: API matches the data model, we 
don't unnecessarily increase memory usage, we avoid duplicating RefSlab type.  
Disadvantage: possibly complex template in a in important clangd API, we 
avoided duplicating at most 100 lines, but we still have to plumb the new slab 
everywhere in the code.




Comment at: clang-tools-extra/clangd/index/FileIndex.h:125
 
+using SlabTuple = std::tuple;
+

AllSlabs?  And even better, a struct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58880



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

[PATCH] D59083: [clangd] Store explicit template specializations in index for code navigation purposes

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clangd/CodeComplete.cpp:1513
 
+template  bool explicitTemplateSpecialization(const NamedDecl &ND) {
+  if (const auto *TD = dyn_cast(&ND))

isExplicitTemplateSpecialization?



Comment at: unittests/clangd/SymbolCollectorTests.cpp:395
   Annotations Header(R"(
-// Template is indexed, specialization and instantiation is not.
-template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
-template <> struct Tmpl {};
-extern template struct Tmpl;
-template struct Tmpl;
+// Template and explicit specialization is indexed, instantiation is not.
+template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};

"Template" -> "Primary template"

"is indexed" -> "are indexed"


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59083



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

[PATCH] D58814: [clang][Index] Constructors and Destructors do not reference class

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added a comment.

Do we also need to change anything for constructor calls?  If not, please add a 
test for that.




Comment at: lib/Index/IndexDecl.cpp:251
+   Ctor->getParent(), Ctor->getDeclContext(),
+   (unsigned)(SymbolRole::NameReference));
 

No need for parens around `SymbolRole::NameReference`.



Comment at: lib/Index/IndexDecl.cpp:268
+ Dtor->getParent(), Dtor->getDeclContext(),
+ (unsigned)(SymbolRole::NameReference));
   }

No need for parens around `SymbolRole::NameReference`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58814



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

[PATCH] D59086: [clangd] Adjust compile commands to be applicable for tooling

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clangd/GlobalCompilationDatabase.cpp:24
  llvm::StringRef ResourceDir) {
+  // Clangd does not generate dependency file.
+  Cmd.CommandLine = tooling::getClangStripDependencyFileAdjuster()(

Please don't duplicate code in comments. Explain why it shouldn't generate the 
dependency files, or drop the comment.





Comment at: clangd/GlobalCompilationDatabase.h:116
+  /// Adjusts given compile command for clangd.
+  tooling::CompileCommand adjustArguments(tooling::CompileCommand Cmd) const;
+

kadircet wrote:
> hokein wrote:
> > ioeric wrote:
> > > This doesn't seem to be used in this patch (except for tests). Could you 
> > > include intended uses in the patch so we can understand the problem 
> > > better?
> > Looks like this patch introduces two changes:
> > 
> > - move the internal adjustArguments to public, I have the same question, 
> > any reason doing it? because we can test it? 
> > - add additional Adjusters to adjustArguments
> It was only for testing
This patch also changes `getCompileCommand` to call `adjustArguments`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59086



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

[PATCH] D59086: [clangd] Adjust compile commands to be applicable for tooling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clangd/GlobalCompilationDatabase.cpp:24
  llvm::StringRef ResourceDir) {
+  // Clangd does not generate dependency file.
+  Cmd.CommandLine = tooling::getClangStripDependencyFileAdjuster()(

kadircet wrote:
> gribozavr wrote:
> > Please don't duplicate code in comments. Explain why it shouldn't generate 
> > the dependency files, or drop the comment.
> > 
> > 
> I am saying we are stripping "dependecy file related" commands, because 
> clangd is not producing them. 
> I feel like this provides the cause of the following statement, not 
> replicates it. Becuase the following statement just says strip "dependency 
> file related" commands.
> 
> But happy to drop the comment if you insist.
What about "clangd should not write files to disk, including dependency files 
requested on the command line."


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59086



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

[PATCH] D59083: [clangd] Store explicit template specializations in index for code navigation purposes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.



Comment at: clangd/CodeComplete.cpp:1613
   };
+  // We only complete symbol's name, which is same as the class template in the
+  // case of template specializations.

which is the same as the name of the *primary* template in case of template 
specializations.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59083



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

[PATCH] D58815: [clangd] Make sure constructors do not reference class

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.



Comment at: clangd/XRefs.cpp:160
+if (Roles & static_cast(index::SymbolRole::NameReference))
+  return true;
 if (Loc == SearchedLocation) {

Maybe add a blank line so that it is clear that the comment is only attached to 
the if stmt?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58815



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

[PATCH] D59128: [clangd] Redirect clangd page.

[Dmitri Gribenko via Phabricator via cfe-commits]

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

Fancy!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59128



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:266
+
+  /// Returns the AST statement representing OpenMP structured-block of this
+  /// OpenMP executable directive,

"the AST node"



Comment at: include/clang/AST/StmtOpenMP.h:2158
 
+  /// OpenMP flush construct is a stand-alone directive.
+  llvm::Optional getStructuredBlockImpl() const { return llvm::None; };

This comment (and other similar comments on getStructuredBlockImpl()) are 
implementation comments and should be written in the function body.



Comment at: include/clang/Basic/OpenMPKinds.def:18
+#ifndef OPENMP_EXECUTABLE_DIRECTIVE
+#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class)
+#endif

Why not add a default definition:

```
#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class) OPENMP_DIRECTIVE(Name)
```

(Also for `OPENMP_EXECUTABLE_DIRECTIVE_EXT` below.)



Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | 
FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+

I'm not a fan of ast-dump tests.  They are fragile, difficult to update, 
difficult to read (when they are 700 lines long), and it is unclear what the 
important parts are.

WDYT about converting them to unit tests?  See 
`clang/unittests/AST/StmtPrinterTest.cpp` for an example.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:269
+  /// or if this is an OpenMP stand-alone directive returns `None`.
+  llvm::Optional getStructuredBlock() const;
 };

Why not `Stmt *` as a return value?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

I left some comments, but it is difficult for me to review without 
understanding what the requirements for this hasher are, why some information 
is hashed in, and some is left out, could you clarify?  See detailed comments.

> This implementation is covered by lit tests using it through c-index-test in 
> upcoming patch.

This code looks very unit-testable.  It also handles many corner cases (what 
information is hashed and what is left out).  c-index-tests are integration 
tests that are not as good at that, and also they would be testing this code 
quite indirectly.




Comment at: clang/lib/Index/IndexRecordHasher.cpp:175
+// There's a balance between caching results and not growing the cache too
+// much. Measurements showed that avoiding caching all decls is beneficial
+// particularly when including all of Cocoa.

"caching all decls" => "caching hashes for all decls"



Comment at: clang/lib/Index/IndexRecordHasher.cpp:191
+
+  auto asCanon = [](QualType Ty) -> CanQualType {
+return CanQualType::CreateUnsafe(Ty);

I don't think that hiding a "CreateUnsafe" in an operation with a benign name 
"asCanon" is a good idea.  Why not inline this lambda everywhere, it looks like 
it is defined to only save typing a few characters.



Comment at: clang/lib/Index/IndexRecordHasher.cpp:207
+if(qVal)
+  Hash = hash_combine(Hash, qVal);
+

What about other qualifiers?

Why not use `Qualifiers::getAsOpaqueValue()` instead of manually converting a 
subset of qualifiers to unsigned?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:209
+
+// Hash in ObjC GC qualifiers?
+

Is this a FIXME or?..



Comment at: clang/lib/Index/IndexRecordHasher.cpp:215
+if (const PointerType *PT = dyn_cast(T)) {
+  Hash = hash_combine(Hash, '*');
+  CT = asCanon(PT->getPointeeType());

Why not hash in `T->getTypeClass()` uniformly for all types, instead of 
inventing a random sigil?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:219
+}
+if (const ReferenceType *RT = dyn_cast(T)) {
+  Hash = hash_combine(Hash, '&');

There is LValueReferenceType and RValueReferenceType, do we care about the 
difference?  If not, why not?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:254
+
+break;
+  }

What about all other types -- array type, function type, decltype(), ...?

What about attributes?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:277
+template 
+hash_code IndexRecordHasher::tryCache(const void *Ptr, T Obj) {
+  auto It = HashByPtr.find(Ptr);

I had to read the implementation of this function to understand what it does.  
How about renaming it to "cachedHash()" ?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:291
+
+hash_code IndexRecordHasher::hashImpl(const Decl *D) {
+  return DeclHashVisitor(*this).Visit(D);

hashImpl => cachedHashImpl



Comment at: clang/lib/Index/IndexRecordHasher.cpp:409
+
+  // Unhandled type.
+  return Hash;

So what is the failure mode for unhandled types, what is the effect on the 
whole system?



Comment at: clang/lib/Index/IndexRecordHasher.cpp:476
+  case NestedNameSpecifier::TypeSpecWithTemplate:
+// Fall through to hash the type.
+

I'd suggest to remove this comment.  It is more confusing than helpful.  It 
makes the code look like there's some processing (like a "break" in other 
cases), but at a close inspection it turns out to be a comment.  This kind of 
fallthrough is pretty common and I don't think it requires a comment.  (For 
example, hashImpl above has this kind of fallthough, but does not have comments 
like this.)



Comment at: clang/lib/Index/IndexRecordHasher.h:31
+/// Implements hashing of AST nodes suitable for the index.
+/// Caching all produced hashes.
+class IndexRecordHasher {

Could you add some explanation about the information that is being hashed?  
What is the underlying principle behind choosing to hash some aspect of the AST 
node (or to skip it).  For example, I see you're hashing names, but not, say 
source locations.  Or that QualTypes are first translated into canonical types.

What are the completeness constraints for this hasher?  What happens if we 
don't hash something that we should have?

"Caching all produced hashes" seems like an implementation comment.  Especially 
since the implementation chooses not to cache some of the hashes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58749



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.o

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/AST/StmtOpenMP.h:269
+  /// or if this is an OpenMP stand-alone directive returns `None`.
+  llvm::Optional getStructuredBlock() const;
 };

lebedev.ri wrote:
> gribozavr wrote:
> > Why not `Stmt *` as a return value?
> Good question.
> 
> Because usually checking a pointer for null is an exceptional
> case, you //normally// have the AST node, you don't check
> **every single** pointer for `null`, i don't think?
> 
> This is absolutely not the case here,  `null` is **not** an
> exceptional failure state here, it's the expected return value
> for stand-alone directives. With `Optional`, you explicitly
> signal "hey, i'm not a plain pointer, pay attention!",
> thus hopefully maybe preventing some issues.
> 
> I'm not sure we can more nicely solve this with an extra base class[es],
> at least not without having only two direct subclasses of 
> `OMPExecutableDirective`:
> * `OMPNonstandaloneExecutableDirective`.
> * `OMPStandaloneExecutableDirective`.
> and two different `OMPOrderedDirective` classes (one standalone, one not).
> But somehow i don't think there is any chance of that being accepted,
> even though it would result in slightly cleaner AST.
> Because usually checking a pointer for null is an exceptional
> case, you normally have the AST node, you don't check
> every single pointer for null, i don't think?

When deciding whether to check for null, we look at the function contract and 
check those pointers that are nullable.  For example, `IfStmt::getThen()` does 
not return NULL in valid ASTs, but `IfStmt::getElse()` does.

> This is absolutely not the case here,  null is not an
> exceptional failure state here, it's the expected return value
> for stand-alone directives.

Yes, I understand what optional is and how a new codebase could use it with 
pointers.  However, LLVM and Clang code has not been using optional to indicate 
nullability for pointers.  I could only find 136 occurrences of 
`Optional<.*\*>` in llvm-project.git.



Comment at: include/clang/Basic/OpenMPKinds.def:18
+#ifndef OPENMP_EXECUTABLE_DIRECTIVE
+#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class)
+#endif

lebedev.ri wrote:
> gribozavr wrote:
> > Why not add a default definition:
> > 
> > ```
> > #  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class) OPENMP_DIRECTIVE(Name)
> > ```
> > 
> > (Also for `OPENMP_EXECUTABLE_DIRECTIVE_EXT` below.)
> Hm, i have never seen that chaining pattern in `.def` headers before,
> so it could be surprising behavior.
It is used in Clang, for example, in 
`llvm/tools/clang/include/clang/AST/TypeNodes.def`, or 
`llvm/tools/clang/include/clang/AST/BuiltinTypes.def`.  I was surprised that 
this file does not use this pattern, to be honest.



Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | 
FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+

lebedev.ri wrote:
> gribozavr wrote:
> > I'm not a fan of ast-dump tests.  They are fragile, difficult to update, 
> > difficult to read (when they are 700 lines long), and it is unclear what 
> > the important parts are.
> > 
> > WDYT about converting them to unit tests?  See 
> > `clang/unittests/AST/StmtPrinterTest.cpp` for an example.
> > They are difficult to update.
> 
> The updating part is true, for all of the clang tests,
> they unlike llvm tests have no update scripts, so it's a major pain.
> Here, it's is actually reasonably simple:
> 1. prune old `// CHECK*` lines
> 2. run the run line
> 3. prepend each line of output with `// CHECK-NEXT: `
> 4. scrub addresses, `s/0x[0-9a-f]+/0x{{.*}}/`
> 5. scrub filepath prefix, again a simple string-replace
> 6. prune everything after `TranslationUnitDecl` and before first 
> `FunctionDecl`
> 7. replace a few `// CHECK-NEXT: ` with `// CHECK: `
> 8. append the final processed output to the test file. done.
> 
> It would not be too hard to write an update tool for this, but i've managed 
> with a simple macro..
> 
> > They are fragile, difficult to read (when they are 700 lines long),
> > and it is unclear what the important parts are.
> 
> This is kind-of intentional. I've tried to follow the llvm-proper approach of 
> gigantic 
> tests that are overly verbose, but quickly cement the state so **any** change 
> **will**
> be noticed. That has been a **very** successful approach for LLVM.
> 
> In fact, the lack of this ast-dumper test coverage was not helping
> the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> 
> So no, i really don't want to //convert// the tests into a less verbose state.
> 
> I suppose i //could// //add// a more fine-grained tests, 
> though readability is arguable. Here, it is obvious nothing is omitted,
> and you can quickly see the `openmp_structured_block` by Ctrl+F'ing for it.
> With more distilled tests, it's less obvious

[PATCH] D58749: [index-while-building] IndexRecordHasher

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> I basically didn't really like the idea of testing against hard-coded hash 
> values in unittests as I consider it to be an implementation detail.

Sorry, that's not what I was suggesting.  There are better ways to test 
hashing.  For example, write a piece of source code and check that the hash 
codes for two declarations that only differ in one aspect are same or different.

For example,

  void f(int &&);
  void f(int &);

and then assert that hashes for two decls are same or different depending on 
the desired specification.

> I was thinking about integration tests that would work around this by both 
> writing index and reading index and writing assertions against that data.

How is hashing used in this process?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58749



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | 
FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > I'm not a fan of ast-dump tests.  They are fragile, difficult to 
> > > > update, difficult to read (when they are 700 lines long), and it is 
> > > > unclear what the important parts are.
> > > > 
> > > > WDYT about converting them to unit tests?  See 
> > > > `clang/unittests/AST/StmtPrinterTest.cpp` for an example.
> > > > They are difficult to update.
> > > 
> > > The updating part is true, for all of the clang tests,
> > > they unlike llvm tests have no update scripts, so it's a major pain.
> > > Here, it's is actually reasonably simple:
> > > 1. prune old `// CHECK*` lines
> > > 2. run the run line
> > > 3. prepend each line of output with `// CHECK-NEXT: `
> > > 4. scrub addresses, `s/0x[0-9a-f]+/0x{{.*}}/`
> > > 5. scrub filepath prefix, again a simple string-replace
> > > 6. prune everything after `TranslationUnitDecl` and before first 
> > > `FunctionDecl`
> > > 7. replace a few `// CHECK-NEXT: ` with `// CHECK: `
> > > 8. append the final processed output to the test file. done.
> > > 
> > > It would not be too hard to write an update tool for this, but i've 
> > > managed with a simple macro..
> > > 
> > > > They are fragile, difficult to read (when they are 700 lines long),
> > > > and it is unclear what the important parts are.
> > > 
> > > This is kind-of intentional. I've tried to follow the llvm-proper 
> > > approach of gigantic 
> > > tests that are overly verbose, but quickly cement the state so **any** 
> > > change **will**
> > > be noticed. That has been a **very** successful approach for LLVM.
> > > 
> > > In fact, the lack of this ast-dumper test coverage was not helping
> > > the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> > > 
> > > So no, i really don't want to //convert// the tests into a less verbose 
> > > state.
> > > 
> > > I suppose i //could// //add// a more fine-grained tests, 
> > > though readability is arguable. Here, it is obvious nothing is omitted,
> > > and you can quickly see the `openmp_structured_block` by Ctrl+F'ing for 
> > > it.
> > > With more distilled tests, it's less obvious. (i'm not talking about 
> > > `StmtPrinterTest`)
> > > Here, it's is actually reasonably simple:
> > 
> > Unfortunately, an 8-step process is anything but simple.
> > 
> > > This is kind-of intentional. I've tried to follow the llvm-proper 
> > > approach of gigantic 
> > > tests that are overly verbose, but quickly cement the state so any change 
> > > will
> > > be noticed.
> > 
> > That's pretty much a definition of a "fragile test".  The tests should 
> > check what they intend to check.
> > 
> > > That has been a very successful approach for LLVM.
> > 
> > I don't think LLVM does this, the CHECK lines are for the most part 
> > manually crafted.
> > 
> > > In fact, the lack of this ast-dumper test coverage was not helping
> > > the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.
> > 
> > If we need tests for AST dumper, we should write tests for it.  OpenMP 
> > tests should not double for AST dumper tests.
> > 
> >> Here, it's is actually reasonably simple:
> > Unfortunately, an 8-step process is anything but simple.
> 
> Hah, what i was saying is that it is entirely automatable,
> all steps are identical each time.
> 
> > That's pretty much a definition of a "fragile test". The tests should check 
> > what they intend to check.
> > I don't think LLVM does this, the CHECK lines are for the most part 
> > manually crafted.
> 
> No.
> I //know// that 'most' of the new/updated tests are not manual,
> from looking at the tests in commits, it's not an empty statement.
> 
> ```
> llvm/test/CodeGen$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname 
> \*.ll | xargs grep "update_llc" | wc -l; done | sort -r -n -k 2,2
> X86 1427
> PowerPC 125
> RISCV 90
> ...
> ```
> ```
> llvm/test/CodeGen$ find X86/ -iname *.ll | wc -l
> 3370
> llvm/test/CodeGen$ find PowerPC/ -iname *.ll | wc -l
> 1019
> llvm/test/CodeGen$ find RISCV/ -iname *.ll | wc -l
> 105
> ```
> So a half of X86 backend tests, and pretty much all RISCV backend tests.
> ```
> llvm/test$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | 
> xargs grep "update_test" | wc -l; done | sort -r -n -k 2,2
> Transforms 845
> Analysis 17
> CodeGen 6
> Other 2
> ```
> ```
> $ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | wc -l; done | 
> sort -r -n -k 2,2
> Transforms 4620
> Analysis 804
> ```
> So sure, by the numbers, 'most' aren't auto-generated.
> It's a question of legacy mainly.
> 
> I've both added llvm tests, and clang tests.
> llvm tests are a breeze, just come up with sufficient IR,
> and run the script [veri

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Sorry for the delay.  I will finish reviewing tomorrow.




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:9
+/// \file
+/// \brief Utility class for listening for file system changes in a directory.
+//===--===//

This comment only repeats the doc comment for the class, I'd suggest to remove 
it here.


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

https://reviews.llvm.org/D58418



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

[PATCH] D59336: [clang-tidy] Disable google-runtime-int in Objective-C++ 🔓

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp:57
   // Find all TypeLocs. The relevant Style Guide rule only applies to C++.
-  if (!getLangOpts().CPlusPlus)
+  // This check is also not applied in Objective-C++ sources as Objective-C
+  // often uses built-in integer types other than `int`.

"Objective-C and Objective-C++"?  Similarly in release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59336



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

[PATCH] D59336: [clang-tidy] Disable google-runtime-int in Objective-C++ 🔓

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/google/IntegerTypesCheck.cpp:57
   // Find all TypeLocs. The relevant Style Guide rule only applies to C++.
-  if (!getLangOpts().CPlusPlus)
+  // This check is also not applied in Objective-C++ sources as Objective-C
+  // often uses built-in integer types other than `int`.

gribozavr wrote:
> "Objective-C and Objective-C++"?  Similarly in release notes.
I see -- should have read the previous sentence, which you are not changing.  
Never mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59336



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:27
+/// be invoked even if the directory is empty.
+class DirectoryWatcher {
+public:

I feel like since there's nothing Clang-specific about it, it should go into 
libSupport in LLVM, what do you think?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:33
+/// A file gets moved into the directory and replaces an existing file
+/// with the same name will trigger an 'Added' event but no 'Removed' 
event.
+/// If a file gets replaced multiple times within a short time period, it

"A file being moved into ... and replacing ... "



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:43
+Removed,
+/// A file was modified.
+Modified,

"File content was modified." ?  In other words, metadata was not modified?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:46
+/// The watched directory got deleted. No more events will follow.
+DirectoryDeleted,
+  };

`DirectoryRemoved` (for consistency with `Removed`)

Also, how about `TopDirectoryRemoved`?  Otherwise it sounds like some random 
directory was removed.



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:51
+EventKind Kind;
+std::string Filename;
+  };

Please document if it is going to be absolute or relative path, or just the 
file name.




Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:55
+  typedef std::function Events, bool isInitial)>
+  EventReceiver;
+

Users are not going to use this typedef in their code, so I feel like it is 
obfuscating code -- could you expand it in places where it is used?



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:61
+  EventReceiver Receiver,
+  bool waitInitialSync,
+  std::string &Error);

`WaitForInitialSync` (everywhere in the patch)



Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:62
+  bool waitInitialSync,
+  std::string &Error);
+

Why not return `llvm::ErrorOr>`?

I also don't understand why we need unique_ptr.  If you change `Implementation 
&Impl;` to `std::unique_ptr Impl;`, `DirectoryWatcher` becomes 
a move-only type, and `create` can return `llvm::ErrorOr`.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:1
+//===- DirectoryWatcher-linux.inc.h - Linux-platform directory listening 
--===//
+//

Please drop the `.h` from the name, for consistency with the rest of LLVM 
(there are no `.inc.h` files, only `.inc`).



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:93
+  30 * (sizeof(struct inotify_event) + NAME_MAX + 1);
+  char buf[EventBufferLength] __attribute__((aligned(8)));
+

Please use `AlignedCharArray` from `llvm/include/llvm/Support/AlignOf.h`.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:93
+  30 * (sizeof(struct inotify_event) + NAME_MAX + 1);
+  char buf[EventBufferLength] __attribute__((aligned(8)));
+

gribozavr wrote:
> Please use `AlignedCharArray` from `llvm/include/llvm/Support/AlignOf.h`.
NAME_MAX is 255, add some for inotify_event, and multiply by 30... we get 
almost 8 Kb on the stack.  Should we allocate on the heap instead?



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:137
+
+bool DirectoryWatcher::Implementation::initialize(StringRef Path,
+  EventReceiver Receiver,

Return `llvm::Error`?



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:162
+  std::thread watchThread(
+  std::bind(runWatcher, pathToWatch, inotifyFD, evtQueue));
+  watchThread.detach();

Use a lambda instead of bind to improve readability?  
https://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-bind.html



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:175
+std::thread scanThread(runScan);
+scanThread.detach();
+  }

Same comments as for macOS: we need to buffer events from inotify until the 
initial scan completes.



Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-mac.inc.h:1
+//===- DirectoryWatcher-mac.inc.h - Mac-platform director

[PATCH] D59407: [clangd] Add RelationSlab

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang-tools-extra/clangd/index/Index.cpp:35
+auto *Array = Arena.Allocate(Rels.size());
+std::uninitialized_copy(Rels.begin(), Rels.end(), Array);
+Result.emplace_back(Entry.first,

Use `ArrayRef::copy()`, for example: https://reviews.llvm.org/D58782




Comment at: clang-tools-extra/clangd/index/Index.h:43
+public:
+  using value_type = std::pair>;
+  using const_iterator = std::vector::const_iterator;

`struct Relation`?  And in the comments for it, please explain which way the 
relationship is directed (is the SymbolID in the key the subtype?  or is the 
SymbolID in the ArrayRef the subtype?).



Comment at: clang-tools-extra/clangd/index/Index.h:88
+  size_t NumRelations = 0;
+};
+

Please move all new declarations into `Relation.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407



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

[PATCH] D59407: [clangd] Add RelationSlab

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/index/Index.h:59
+return sizeof(*this) + Arena.getTotalMemory() +
+   sizeof(value_type) * Relations.size();
+  }

nridge wrote:
> kadircet wrote:
> > use capacity instead of size
> Note, `RefSlab::bytes()` (which I where I copied this from) uses `size()` as 
> well. Should I change that too?
I'd say yes -- in a separate patch though.  Thanks for catching it!



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

"--- Relation.h "



Comment at: clang-tools-extra/clangd/index/Relation.h:41
+public:
+  // Key.Symbol is Key.Kind of every symbol in Value.
+  // For example, if Key.Kind == SymbolRole::RelationChildOf,

Three slashes for doc comments, please.



Comment at: clang-tools-extra/clangd/index/Relation.h:45
+  // in Value are the base classes of Key.Symbol).
+  struct Relation {
+RelationKey Key;

Lift it up into the `clang::clangd` namespace?  (like `Symbol` and `Ref`)



Comment at: clang-tools-extra/clangd/index/Relation.h:68
+
+  // RelationSlab::Builder is a mutable container that can 'freeze' to
+  // RelationSlab.

No need to repeat the type name being documented.  "A mutable container that 
can ..."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6390
+/// Given OpenMP directive, matches the first clause (out of all specified),
+/// that matches InnerMatcher.
+///

Other comments usually don't explain the interactions with inner matchers, 
unless the semantics are unusual.

"Matches any clause in an OpenMP directive."



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6400
+/// \endcode
+AST_MATCHER_P(OMPExecutableDirective, hasClause, internal::Matcher,
+  InnerMatcher) {

Looking at other similar matches, they usually follow the naming pattern 
`hasAny~`, WDYT?

(`hasAnyDeclaration`, `hasAnyConstructorInitializer` etc.)



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6411
+///
+/// Given, `ompDefaultClause()`:
+///

Please follow the existing comment style.  Either:

```
Given

\code

\endcode

 matches "".
```

or

```
Example:  matches "" in 

\code

\endcode   
```

For example:

```
Given

 \code
   #pragma omp parallel default(none)
   #pragma omp parallel default(shared)
   #pragma omp parallel
 \endcode

``ompDefaultClause()`` matches ``default(none)` and ``default(shared)``.
```

Similarly for other comments in this patch.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6433
+  return Node.getDefaultKind() == OMPC_DEFAULT_none;
+}
+

Also add `isSharedKind`?  It is weird that we have one but not the other.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6449
+///
+/// NOTE: while the matcher takes the *matcher* for the OpenMP clause,
+///   it does *NOT* actually match that matcher. It only fetches the

"while this matcher"



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum value?

I don't see right now advantages for taking a matcher.  (For example, it can't 
be a more complex matcher with inner matchers, it can't be a disjunction of 
matchers etc.)



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

I'm not sure if breaking out the source code into the "SourceX" variables 
improves readability.  WDYT about inlining the code into the EXPECT_TRUE code 
like in other tests in this file?

If you want to break it out, I'd suggest to drop "`void x() {`" down to the 
next line, so that all code lines start at the same column.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> gribozavr wrote:
> > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum 
> > value?
> > 
> > I don't see right now advantages for taking a matcher.  (For example, it 
> > can't be a more complex matcher with inner matchers, it can't be a 
> > disjunction of matchers etc.)
> I don't feel like it, it's uglier.
> The matcher is documented, `OpenMPClauseKind` is not documented.
> Also, how will passing some random enum work with e.g. clang-query?
> 
There are dozens of clauses in `OpenMPClauseKind`.  We would have to replicate 
them all as matchers to provide a useful API.

> Also, how will passing some random enum work with e.g. clang-query?

See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

lebedev.ri wrote:
> gribozavr wrote:
> > I'm not sure if breaking out the source code into the "SourceX" variables 
> > improves readability.  WDYT about inlining the code into the EXPECT_TRUE 
> > code like in other tests in this file?
> > 
> > If you want to break it out, I'd suggest to drop "`void x() {`" down to the 
> > next line, so that all code lines start at the same column.
> > I'm not sure if breaking out the source code into the "SourceX" variables 
> > improves readability
> 
> It's not about readability. Inlining will break the build, rC354201.
Other tests in this file use string concatenation, see 
`TEST(DeclarationMatcher, MatchHasRecursiveAllOf)` for example.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D59463: [ASTMatchers][OpenMP] OpenMP Structured-block-related matchers

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6389
 
+/// Given OpenMP directive, matches if it is an OpenMP standalone directive,
+/// i.e. an executable directive with no structured block.

"Matches standalone OpenMP directives, i.e., directives that can't have a 
structured block."



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6400
+///   #pragma omp taskyield  // <- match, is standalone
+/// \endcode
+AST_MATCHER(OMPExecutableDirective, isStandaloneDirective) {

Please follow the existing comment style. Either:

```
Given

\code

\endcode

 matches "".
```

or

```
Example:  matches "" in 

\code

\endcode
For example:

Given

 \code
   #pragma omp parallel default(none)
   #pragma omp parallel default(shared)
   #pragma omp parallel
 \endcode

``ompDefaultClause()`` matches ``default(none)` and ``default(shared)``.
```

Similarly for other comments in this patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59463



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum 
> > > > value?
> > > > 
> > > > I don't see right now advantages for taking a matcher.  (For example, 
> > > > it can't be a more complex matcher with inner matchers, it can't be a 
> > > > disjunction of matchers etc.)
> > > I don't feel like it, it's uglier.
> > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > Also, how will passing some random enum work with e.g. clang-query?
> > > 
> > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > replicate them all as matchers to provide a useful API.
> > 
> > > Also, how will passing some random enum work with e.g. clang-query?
> > 
> > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> True. Also, but there's dosens of Stmt types, and there is no overload that 
> takes `StmtClass` enum.
For Stmts, we do have dozens of individual matchers for them.

The point of your work is to add ASTMatchers for OpenMP, right?  However, if 
there are no matchers for a reasonable amount of AST surface, it is as good as 
if the matchers are not there, because prospective users won't be able to use 
them.

I don't particularly care how exactly this is achieved, through individual 
matchers or through a matcher that takes an enum.  However, I want to make sure 
that if you're going through all this trouble to add matchers, the resulting 
API should cover a good amount of AST.

The reason why I suggested to pass the enum to the matcher is simply because it 
is less code duplication, less work, and more reliable code (since there will 
be only one matcher to review, test, and maintain, instead of combinations of 
matchers).

Another reason to not use an inner matcher here is the peculiar semantics of 
this function -- it does not evaluate the matcher, and it does not accept a 
matcher expression of any shape.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > I'm not sure if breaking out the source code into the "SourceX" 
> > > > variables improves readability.  WDYT about inlining the code into the 
> > > > EXPECT_TRUE code like in other tests in this file?
> > > > 
> > > > If you want to break it out, I'd suggest to drop "`void x() {`" down to 
> > > > the next line, so that all code lines start at the same column.
> > > > I'm not sure if breaking out the source code into the "SourceX" 
> > > > variables improves readability
> > > 
> > > It's not about readability. Inlining will break the build, rC354201.
> > Other tests in this file use string concatenation, see 
> > `TEST(DeclarationMatcher, MatchHasRecursiveAllOf)` for example.
> I'm sorry, but i fail to see how that is relevant?
> I'm using multiline raw string literals, and inlining it will break the 
> build, like i linked.
> You are pointing at the code that is not using multiline raw string literals.
> You only suggested inlining, not switching away from multiline raw string 
> literals, i believe?
> 
> Not using multiline raw string literals looked even worse, because then you 
> need to manually add "\n"
> You only suggested inlining, not switching away from multiline raw string 
> literals, i believe?

I am now suggesting to switch away from raw string literals.

> Not using multiline raw string literals looked even worse, because then you 
> need to manually add "\n"

I believe that adding "\n" manually is better than having lots of 
similarly-named SourceX variables, which can easily cause copy-paste mistakes 
(define a SourceX variable, use SourceY in the EXPECT_TRUE line).

However, this is a minor point, up to you.  I only wanted to explain my 
reasoning why I prefer inline code snippets.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/AST/Stmt.h:102
+/// This bit is set only for the Stmt's that are the structured-block
+/// of OpeMP [non-standalone] executable directives.
+/// I.e. those returned by OMPExecutableDirective::getStructuredBlock().

"non-standalone OpenMP executable directives"



Comment at: test/PCH/stmt-openmp_structured_block-bit.cpp:8
+
+// expected-no-diagnostics
+

Maybe try imitating a more recently written test, like 
`llvm/tools/clang/test/PCH/cxx2a-compare.cpp` ?  It has both header and user 
written in the same file, for improved readability.



Comment at: test/PCH/stmt-openmp_structured_block-bit.h:2
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/cxx_exprs.h -std=c++11 -fsyntax-only -verify %s 
-ast-dump | FileCheck %s
+

Is cxx_exprs.h needed?

I don't think `-std=c++11` is needed either.

Same in RUN lines below.

Maybe try imitating a more recently written test, like 
`llvm/tools/clang/test/PCH/cxx2a-compare.cpp` ?



Comment at: test/PCH/stmt-openmp_structured_block-bit.h:5
+// Test with pch. Use '-ast-dump' to force deserialization of function bodies.
+// RUN: %clang_cc1 -x c++-header -std=c++11 -emit-pch -o %t %S/cxx_exprs.h
+// RUN: %clang_cc1 -std=c++11 -include-pch %t -fsyntax-only -verify %s 
-ast-dump-all | FileCheck %s

The point of PCH tests is to test the serialization of ASTs into PCH.  In this 
line the cxx_exprs.h header is being initialized, it does not contain any 
OpenMP constructs, so I'm not sure what this test is supposed to test.



Comment at: test/PCH/stmt-openmp_structured_block-bit.h:6
+// RUN: %clang_cc1 -x c++-header -std=c++11 -emit-pch -o %t %S/cxx_exprs.h
+// RUN: %clang_cc1 -std=c++11 -include-pch %t -fsyntax-only -verify %s 
-ast-dump-all | FileCheck %s
+

I don't see any CHECK lines for FileCheck.  (FileCheck fails the test if there 
are no CHECK lines.)



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:9
+//
+// Fine-grained tests for IsOMPStructuredBlock bit of Stmt's.
+//

"of Stmt"



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:27
+
+AST_MATCHER(Stmt, isOMPStructuredBlock) { return Node.isOMPStructuredBlock(); }
+

Eventually you'll move it to `ASTMatchers.h`, right?



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:41
+
+StatementMatcher OMPStandaloneDirectivekMatcher() {
+  return stmt(ompExecutableDirective(isStandaloneDirective())).bind("id");

extra "k" before "Matcher"



Comment at: unittests/AST/PrintTest.h:15
+
+using namespace clang;
+using namespace ast_matchers;

Please don't write `using namespace` in headers.  Instead, put the code below 
into the `clang` namespace.



Comment at: unittests/AST/PrintTest.h:19
+
+namespace {
+

No need for this, make `PrintStmt()` static.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > lebedev.ri wrote:
> > > > > gribozavr wrote:
> > > > > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` 
> > > > > > enum value?
> > > > > > 
> > > > > > I don't see right now advantages for taking a matcher.  (For 
> > > > > > example, it can't be a more complex matcher with inner matchers, it 
> > > > > > can't be a disjunction of matchers etc.)
> > > > > I don't feel like it, it's uglier.
> > > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > > 
> > > > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > > > replicate them all as matchers to provide a useful API.
> > > > 
> > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > 
> > > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > > True. Also, but there's dosens of Stmt types, and there is no overload 
> > > that takes `StmtClass` enum.
> > For Stmts, we do have dozens of individual matchers for them.
> > 
> > The point of your work is to add ASTMatchers for OpenMP, right?  However, 
> > if there are no matchers for a reasonable amount of AST surface, it is as 
> > good as if the matchers are not there, because prospective users won't be 
> > able to use them.
> > 
> > I don't particularly care how exactly this is achieved, through individual 
> > matchers or through a matcher that takes an enum.  However, I want to make 
> > sure that if you're going through all this trouble to add matchers, the 
> > resulting API should cover a good amount of AST.
> > 
> > The reason why I suggested to pass the enum to the matcher is simply 
> > because it is less code duplication, less work, and more reliable code 
> > (since there will be only one matcher to review, test, and maintain, 
> > instead of combinations of matchers).
> > 
> > Another reason to not use an inner matcher here is the peculiar semantics 
> > of this function -- it does not evaluate the matcher, and it does not 
> > accept a matcher expression of any shape.
> > The point of your work is to add ASTMatchers for OpenMP, right?
> 
> Absolutely not.
> D57113 + D59466 is the one and only point, to address the bugs i have 
> personally encountered.
> The whole reason why i have started off with NOT adding these matchers to the 
> `ASTMatchers.h`,
> but keeping them at least initially internal to the checks was to avoid all 
> this bikeshedding.
However, I do care about the AST matchers being usable by other clients.

I also care about the API following existing patterns:

> Another reason to not use an inner matcher here is the peculiar semantics of 
> this function -- it does not evaluate the matcher, and it does not accept a 
> matcher expression of any shape.




Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

As far as I'm concerned, I don't have any major revision requests.  I haven't 
reviewed the unit tests (I'm planning to), but for non-test code, I don't have 
any further comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:64
+++i;
+})";
+  ASSERT_TRUE(

Same comment as in the other patch -- I would prefer that the source is inlined 
into the ASSERT_TRUE, with implicit string concatenation and "\n"s, if raw 
strings don't work.



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:75
+  ASSERT_TRUE(PrintedOMPStmtMatches(Source, OMPStandaloneDirectivekMatcher(),
+"#pragma omp barrier\n"));
+}

Could you also add an assertion that `OMPStructuredBlockMatcher()` matches zero 
AST nodes?  Similarly in every test that has no structured blocks.



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:121
+
+TEST(OMPStructuredBlock, TestDistributeParallelForSimdDirective0) {
+  const char *Source =

This test file repeats this pattern many times, only with different OpenMP 
directives:

TEST(OMPStructuredBlock, Test~~~Directive0)
TEST(OMPStructuredBlock, Test~~~Directive1)
TEST(OMPStructuredBlock, Test~~~DirectiveCollapse1)
TEST(OMPStructuredBlock, Test~~~DirectiveCollapse2)
TEST(OMPStructuredBlock, Test~~~DirectiveCollapse22)

WDYT about deduplicating it using parameterized tests?  For example, see 
AssignmentTest in 
`llvm/tools/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp`.



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:656
+
+TEST(OMPStructuredBlock, DISABLED_TestParallelMaster0XFAIL) {
+  const char *Source =

Is it only a pretty-printer problem or something more severe?



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:704
+
+StatementMatcher OMPSectionsDirectivekMatcher() {
+  return stmt(

Extra "k" before "Matcher".



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:712
+
+StatementMatcher OMPSectionDirectivekMatcher() {
+  return stmt(isOMPStructuredBlock(),

Extra "k" before "Matcher".



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:1481
+
+TEST(OMPStructuredBlock, TesteamsDistributeParallelForSimdDirective0) {
+  const char *Source =

"TestTeams"



Comment at: unittests/AST/PrintTest.h:1
+//===- unittests/AST/PrintTest.h 
--===//
+//

"ASTPrint.h"?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:64
+++i;
+})";
+  ASSERT_TRUE(

lebedev.ri wrote:
> gribozavr wrote:
> > Same comment as in the other patch -- I would prefer that the source is 
> > inlined into the ASSERT_TRUE, with implicit string concatenation and "\n"s, 
> > if raw strings don't work.
> I still don't understand the reasoning here.
> It feels like a change just for the sake of the change.
These variables don't improve readability.  Also, if there are multiple such 
variables in one test, it is easy to mistakenly use an incorrect one in the 
ASSERT_TRUE line.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/Stmt.h:102
+/// This bit is set only for the Stmt's that are the structured-block
+/// of OpeMP [non-standalone] executable directives.
+/// I.e. those returned by OMPExecutableDirective::getStructuredBlock().

lebedev.ri wrote:
> gribozavr wrote:
> > "non-standalone OpenMP executable directives"
> English isn't my native language; to me the current variant (attempts to?) 
> convey
> that structured block can only ever exist for non-standalone executable 
> directives.
> I.e. "structured block of non-standalone executable directives" is a misnomer.
> 
> The variant without `[]` does not convey any of that, i think, it just says
> that "it's a structured block of non-standalone OpenMP executable directives",
> it isn't obvious from that (without knowing OpenMP details) that there 
> wouldn't be "a structured block of standalone OpenMP executable directive".
> 
> Isn't there an important difference there?
I didn't catch this difference implied by "[]".

> I.e. "structured block of non-standalone executable directives" is a misnomer.

Why is it a misnomer?  It might be overly verbose, but it is correct.

However, I see that this fact is important to highlight, so I'd suggest to 
spell it out:

"This bit is set only for the Stmts that are the structured-block of OpenMP 
executable directives.  Directives that have a structured block are called 
"non-standalone" directives."

Please also fix the typo "OpeMP" => OpenMP.



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:64
+++i;
+})";
+  ASSERT_TRUE(

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > Same comment as in the other patch -- I would prefer that the source is 
> > > > inlined into the ASSERT_TRUE, with implicit string concatenation and 
> > > > "\n"s, if raw strings don't work.
> > > I still don't understand the reasoning here.
> > > It feels like a change just for the sake of the change.
> > These variables don't improve readability.  Also, if there are multiple 
> > such variables in one test, it is easy to mistakenly use an incorrect one 
> > in the ASSERT_TRUE line.
> Can you please quote something about this from
> https://llvm.org/docs/CodingStandards.html
> https://llvm.org/docs/ProgrammersManual.html
> etc?
> If not this is entirely subjective/stylistic,
> and this was the form that was more readable to me.
> (easier to write, too, those "\n" will look horrible)
> Also, what about the cases with two assertions? (not just the new ones)
> 
> I really don't see how that would be a good change.
There are lots of readability points that are not formalized in the coding 
style, and yet, are followed in the code.

For cases with two assertions we need to have a variable to avoid code 
duplication, so it is OK there -- the variable has a purpose.

If you want to use raw string literals, could I at least ask you to drop the 
first line onto the same level of indentation as the rest of the lines?

```
  const char *Source =
  R"(void test(int i) {
#pragma omp atomic
++i;
})";
```

change to:

```
  const char *Source = R"(
void test(int i) {
#pragma omp atomic
++i;
})";
```



Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D59528: [clang-tidy] Expand modular headers for PPCallbacks

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidy.h:156
+  virtual void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) {}
+

Please document that `ModuleExpanderPP` also provides events from the main file 
(until I read the implementation in the check, I thought the check should 
subscribe to both preprocessors).



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:19
+public:
+  // Stores FileEntry for which contents are to be recorded later.
+  void addFileToRecord(const FileEntry *File) { FilesToRecord.insert(File); }

Three slashes in doc comments (everywhere in the patch).



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:20
+  // Stores FileEntry for which contents are to be recorded later.
+  void addFileToRecord(const FileEntry *File) { FilesToRecord.insert(File); }
+

"Records that a given file entry is needed for replaying callbacks."

"addNecessaryFile"?



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:23
+  // Records content for a file and adds it to the FileSystem.
+  void recordContent(const FileEntry *File,
+ const SrcMgr::ContentCache *ContentCache,

"recordFileContent"?



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:24
+  void recordContent(const FileEntry *File,
+ const SrcMgr::ContentCache *ContentCache,
+ llvm::vfs::InMemoryFileSystem &InMemoryFs) {

`const &` for `File` and `ContentCache` ?



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:38
+   ContentCache->getRawBuffer()->getBuffer()));
+// Remove file since we have successfully recorded its contents.
+FilesToRecord.erase(File);

"remove the file from the set of necessary files..."



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:71
+
+  // Switch of header modules in the new preprocessor.
+  LangOpts.Modules = false;

of => off

However I don't see value in a comment that repeats the source code.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:115
+auto *F = IF.getFile();
+Recorder->addFileToRecord(F);
+  });

`Recorder->addFileToRecord(IF.getFile())`

The intermediate variable doesn't add anything...



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:171
+// Just parse to the corresponding location to generate the same callback for
+// the target_callbacks_.
+void ExpandModularHeadersPPCallbacks::Ident(SourceLocation Loc, StringRef) {

What's `target_callbacks_`?  It is only mentioned in comments.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h:9
+//
+// Defines the ExpandModularHeadersPPCallbacks class that handles #includes of
+// modular headers, traverses all transitively included headers in non-modular

This looks like a documentation comment for the class itself, I think it should 
be moved there.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h:43
+
+  /// \brief Users can get expanded PPCallbacks by registering their callback
+  /// handlers in the preprocessor instance returned by this method.

"Returns the preprocessor that provides callbacks for contents of modular 
headers.

This preprocessor is separate from the one used by the rest of the compiler."




Comment at: 
clang-tools-extra/test/clang-tidy/expand-modular-headers-ppcallbacks.cpp:31
+// CHECK-MESSAGES: c.h:2:9: warning: invalid case style for macro definition 
'c' [readability-identifier-naming]
+// CHECK-MESSAGES: c.h:2:9: note: FIX-IT applied suggested code changes

Please also add a check for a diagnostic coming from the main file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59528



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

[PATCH] D59528: [clang-tidy] Expand modular headers for PPCallbacks

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h:32
+
+/// \brief Handles PPCallbacks and replays preprocessing with modules disabled.
+///

replays => re-runs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59528



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

[PATCH] D59650: [NFC] ExceptionEscapeCheck: small refactoring

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang-tidy/utils/ExceptionAnalyzer.cpp:208
+template <>
+void ExceptionAnalyzer::analyze(const FunctionDecl *Func,
+ExceptionInfo &ExceptionList) {

I'd suggest to make it a non-template and call it also `analyzeImpl()` and 
return `ExceptionInfo by` value.



Comment at: clang-tidy/utils/ExceptionAnalyzer.cpp:226
+ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::analyzeBoilerplate(const T *Node) {
+  ExceptionInfo ExceptionList;

JonasToth wrote:
> lebedev.ri wrote:
> > Please bikeshed on the name. I don't think this one is good.
> Hmm, `analyzeGeneric`, `analyzeGeneral`, `abstractAnalysis`, 
> `analyzeAbstract`, something good in these?
> 
> Given its private its not too important either ;)
I'd suggest to simplify by changing `analyzeBoilerplate()` into a non-template, 
into this specifically:

```
ExceptionAnalyzer::ExceptionInfo 
ExceptionAnalyzer::filterIgnoredExceptions(ExceptionInfo ExceptionList) {
if (ExceptionList.getBehaviour() == State::NotThrowing ||
  ExceptionList.getBehaviour() == State::Unknown)
return ExceptionList;

  // Remove all ignored exceptions from the list of exceptions that can be
  // thrown.
  ExceptionList.filterIgnoredExceptions(IgnoredExceptions, IgnoreBadAlloc);

  return ExceptionList;
}
```

And then call it in `analyze()`:

```
ExceptionAnalyzer::ExceptionInfo
ExceptionAnalyzer::analyze(const FunctionDecl *Func) {
  return filterIgnoredExceptions(analyzeImpl(Func));
}
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59650



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

[PATCH] D59466: [clang-tidy] openmp-exception-escape - a new check

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:53
+  Finder->addMatcher(ompExecutableDirective(
+ unless(isStandaloneDirective()),
+ hasStructuredBlock(stmt().bind("structured-block")))

Do we need the `unless`?  `hasStructuredBlock()` just won't match standalone 
directives.



Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:65
+  Result.Nodes.getNodeAs("structured-block");
+  assert(StructuredBlock && "Expected to get soe OpenMP Structured Block.");
+

soe => some



Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:69
+  utils::ExceptionAnalyzer::State::Throwing)
+return; // No exceptions appear to escape out of the structured block.
+

appear to => have been proven to



Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:72
+  // FIXME: We should provide more information about the exact location where
+  // the exception is thrown, maybe the full path the exception escapes
+

+1



Comment at: docs/clang-tidy/checks/openmp-exception-escape.rst:21
+
+.. option:: IgnoredExceptions
+

`IgnoredExceptionTypes`?



Comment at: test/clang-tidy/bugprone-exception-escape-openmp.cpp:16
+;
+  // FIXME: this really should be caught by bugprone-exception-escape.
+  // https://bugs.llvm.org/show_bug.cgi?id=41102

Shouldn't the function be marked with `noexcept` for 
`bugprone-exception-escape` to catch it?  It does not know about OpenMP.

Are you suggesting that `bugprone-exception-escape` should subsume your new 
OpenMP-specific check?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59466



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

[PATCH] D59407: [clangd] Add RelationSlab

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



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

gribozavr wrote:
> "--- Relation.h "
Not done?



Comment at: clang-tools-extra/clangd/index/Relation.h:41
+public:
+  // Key.Symbol is Key.Kind of every symbol in Value.
+  // For example, if Key.Kind == SymbolRole::RelationChildOf,

gribozavr wrote:
> Three slashes for doc comments, please.
Not done?



Comment at: clang-tools-extra/clangd/index/Relation.h:45
+  // in Value are the base classes of Key.Symbol).
+  struct Relation {
+RelationKey Key;

nridge wrote:
> gribozavr wrote:
> > Lift it up into the `clang::clangd` namespace?  (like `Symbol` and `Ref`)
> This comment made me realize that I haven't addressed your previous comment 
> properly: I haven't changed `RelationSlab::value_type` from 
> `std::pair>` to `Relation`.
> 
> I tried to make that change this time, and ran into a problem:
> 
> In the rest of the subtypes patch (D58880), one of the things I do is extend 
> the `MemIndex` constructor so that, in addition to taking a symbol range and 
> a ref range, it takes a relation range.
> 
> That constructor assumes that the elements of that range have members of some 
> name - either `first` and `second` (as currently in D58880), or `Key` and 
> `Value`.
> 
> However, that constructor has two call sites. In `MemIndex::build()`, we pass 
> in the slabs themselves as the ranges. So, if we make this change, the field 
> names for that call site will be `Key` and `Value`. However, for the second 
> call site in `FileSymbols::buildIndex()`, the ranges that are passed in are 
> `DenseMap`s, and therefore their elements' field names are necessarily 
> `first` and `second`. The same constructor cannot therefore accept both 
> ranges.
> 
> How do you propose we address this?
> 
>  * Scrap `struct Relation`, and keep `value_type` as `std::pair llvm::ArrayRef>`?
>  * Keep `struct Relation`, but make its fields named `first` and `second`?
>  * Split the constructor of `MemIndex` into two constructors, to accomodate 
> both sets of field names?
>  * Something else?
I guess we should scrap it then.  Kadir, WDYT?



Comment at: clang-tools-extra/clangd/index/Relation.h:68
+
+  // RelationSlab::Builder is a mutable container that can 'freeze' to
+  // RelationSlab.

gribozavr wrote:
> No need to repeat the type name being documented.  "A mutable container that 
> can ..."
Not done?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407



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

[PATCH] D59528: [clang-tidy] Expand modular headers for PPCallbacks

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr marked an inline comment as done.
gribozavr added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:166
+Preprocessor->Lex(CurrentToken);
+}
+

Haha, so the test that I asked to add did catch a bug -- just not the one I 
expected :)



Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h:44
+  /// \brief Returns the preprocessor that provides callbacks for contents of
+  /// modular headers.
+  ///

"... callbacks for the whole translation unit, including the main file, textual 
headers, and modular headers."

Sorry, I wrote the comment before I fully understood what this preprocessor 
does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59528



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

[PATCH] D59466: [clang-tidy] openmp-exception-escape - a new check

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr marked an inline comment as done.
gribozavr added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:53
+  Finder->addMatcher(ompExecutableDirective(
+ unless(isStandaloneDirective()),
+ hasStructuredBlock(stmt().bind("structured-block")))

lebedev.ri wrote:
> gribozavr wrote:
> > Do we need the `unless`?  `hasStructuredBlock()` just won't match 
> > standalone directives.
> *need*? no.
> But it makes zero sense semantically to look for structured block
> without first establishing that it has one.
> Sure, we now check that twice (here, and in `hasStructuredBlock()`), but that 
> is up to optimizer.
> 
> The fact that `hasStructuredBlock()` workarounds the assert in
> `getStructuredBlock()` is only to avoid clang-query crashes,
> it is spelled as much in the docs.
> But it makes zero sense semantically to look for structured block without 
> first establishing that it has one.

IDK, in my mind it makes sense.  "Does a standalone directive have a structured 
block?  No." is a coherent logical chain.



Comment at: test/clang-tidy/bugprone-exception-escape-openmp.cpp:16
+;
+  // FIXME: this really should be caught by bugprone-exception-escape.
+  // https://bugs.llvm.org/show_bug.cgi?id=41102

lebedev.ri wrote:
> gribozavr wrote:
> > Shouldn't the function be marked with `noexcept` for 
> > `bugprone-exception-escape` to catch it?  It does not know about OpenMP.
> > 
> > Are you suggesting that `bugprone-exception-escape` should subsume your new 
> > OpenMP-specific check?
> > Shouldn't the function be marked with noexcept for 
> > bugprone-exception-escape to catch it?
> 
> Right. I meant `noexcept` (or did i drop it and forgot to put it back?).
> But that changed nothing here, this is still an XFAIL.
> 
> > It does not know about OpenMP.
> > Are you suggesting that bugprone-exception-escape should subsume your new 
> > OpenMP-specific check?
> 
> Only the `;` is the structured-block here, so `thrower()` call *should* be 
> caught by `bugprone-exception-escape`.
Ah, I see.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59466



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

[PATCH] D59650: [NFC] ExceptionEscapeCheck: small refactoring

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr marked an inline comment as done.
gribozavr added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tidy/utils/ExceptionAnalyzer.cpp:226
+ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::analyzeBoilerplate(const T *Node) {
+  ExceptionInfo ExceptionList;

lebedev.ri wrote:
> gribozavr wrote:
> > JonasToth wrote:
> > > lebedev.ri wrote:
> > > > Please bikeshed on the name. I don't think this one is good.
> > > Hmm, `analyzeGeneric`, `analyzeGeneral`, `abstractAnalysis`, 
> > > `analyzeAbstract`, something good in these?
> > > 
> > > Given its private its not too important either ;)
> > I'd suggest to simplify by changing `analyzeBoilerplate()` into a 
> > non-template, into this specifically:
> > 
> > ```
> > ExceptionAnalyzer::ExceptionInfo 
> > ExceptionAnalyzer::filterIgnoredExceptions(ExceptionInfo ExceptionList) {
> > if (ExceptionList.getBehaviour() == State::NotThrowing ||
> >   ExceptionList.getBehaviour() == State::Unknown)
> > return ExceptionList;
> > 
> >   // Remove all ignored exceptions from the list of exceptions that can be
> >   // thrown.
> >   ExceptionList.filterIgnoredExceptions(IgnoredExceptions, IgnoreBadAlloc);
> > 
> >   return ExceptionList;
> > }
> > ```
> > 
> > And then call it in `analyze()`:
> > 
> > ```
> > ExceptionAnalyzer::ExceptionInfo
> > ExceptionAnalyzer::analyze(const FunctionDecl *Func) {
> >   return filterIgnoredExceptions(analyzeImpl(Func));
> > }
> > ```
> Hmm not really.
> I intentionally did all this to maximally complicate any possibility of 
> accidentally doing
> something different given diferent entry point (`Stmt` vs `FunctionDecl`).
> Refactoring it that way, via `filterIgnoredExceptions()` increases that risk 
> back.
> (accidentally omit that intermediate function, and ...)
> (accidentally omit that intermediate function, and ...)

... and tests should catch it.  No big drama.

Anyway, it is not as important.  I do think however that complicating the code 
this way is not worth the benefit.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59650



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

[PATCH] D59650: [NFC] ExceptionEscapeCheck: small refactoring

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tidy/utils/ExceptionAnalyzer.cpp:226
+ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::analyzeBoilerplate(const T *Node) {
+  ExceptionInfo ExceptionList;

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > JonasToth wrote:
> > > > > lebedev.ri wrote:
> > > > > > Please bikeshed on the name. I don't think this one is good.
> > > > > Hmm, `analyzeGeneric`, `analyzeGeneral`, `abstractAnalysis`, 
> > > > > `analyzeAbstract`, something good in these?
> > > > > 
> > > > > Given its private its not too important either ;)
> > > > I'd suggest to simplify by changing `analyzeBoilerplate()` into a 
> > > > non-template, into this specifically:
> > > > 
> > > > ```
> > > > ExceptionAnalyzer::ExceptionInfo 
> > > > ExceptionAnalyzer::filterIgnoredExceptions(ExceptionInfo ExceptionList) 
> > > > {
> > > > if (ExceptionList.getBehaviour() == State::NotThrowing ||
> > > >   ExceptionList.getBehaviour() == State::Unknown)
> > > > return ExceptionList;
> > > > 
> > > >   // Remove all ignored exceptions from the list of exceptions that can 
> > > > be
> > > >   // thrown.
> > > >   ExceptionList.filterIgnoredExceptions(IgnoredExceptions, 
> > > > IgnoreBadAlloc);
> > > > 
> > > >   return ExceptionList;
> > > > }
> > > > ```
> > > > 
> > > > And then call it in `analyze()`:
> > > > 
> > > > ```
> > > > ExceptionAnalyzer::ExceptionInfo
> > > > ExceptionAnalyzer::analyze(const FunctionDecl *Func) {
> > > >   return filterIgnoredExceptions(analyzeImpl(Func));
> > > > }
> > > > ```
> > > Hmm not really.
> > > I intentionally did all this to maximally complicate any possibility of 
> > > accidentally doing
> > > something different given diferent entry point (`Stmt` vs `FunctionDecl`).
> > > Refactoring it that way, via `filterIgnoredExceptions()` increases that 
> > > risk back.
> > > (accidentally omit that intermediate function, and ...)
> > > (accidentally omit that intermediate function, and ...)
> > 
> > ... and tests should catch it.  No big drama.
> > 
> > Anyway, it is not as important.  I do think however that complicating the 
> > code this way is not worth the benefit.
> That is kind of the point. The test would catch it if they would already 
> exist.
> If a new entry point is being added, the tests wouldn't be there yet.
> This enforces that every entry point behaves the same way.
> This enforces that every entry point behaves the same way.

Not really -- the new entry point can skip calling `analyzeDispatch` (and roll 
its own analysis) just like it can skip calling `filterIgnoredException()`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59650



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

[PATCH] D59407: [clangd] Add RelationSlab

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Submitting code as it becomes ready is the usual practice here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407



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

[PATCH] D59814: [Testing] Move clangd::Annotations to llvm testing support

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: llvm/include/llvm/Testing/Support/Annotations.h:9
 //
 // Annotations lets you mark points and ranges inside source code, for tests:
 //

Move into a doc comment on the class Annonations?



Comment at: llvm/include/llvm/Testing/Support/Annotations.h:49
 public:
   // Parses the annotations from Text. Crashes if it's malformed.
   Annotations(llvm::StringRef Text);

Three slashes for doc comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59814



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

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:69
+  /// The tokens after preprocessor replacements.
+  llvm::ArrayRef tokens(const TokenBuffer &B) const;
+  /// Tokens that appear in the text of the file, i.e. a name of an object-like

`expandedTokens`?



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:72
+  /// macro or a name, arguments and parentheses of a function-like macro.
+  llvm::ArrayRef macroTokens(const TokenBuffer &B) const;
+  /// The range covering macroTokens().

`invocationTokens` or `macroInvocationTokens`



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:74
+  /// The range covering macroTokens().
+  std::pair macroRange(const TokenBuffer &B,
+   const SourceManager &SM) const;

`invocationSourceRange` or `macroInvocationSourceRange` depending on what you 
choose for the function above.




Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:83
+  unsigned BeginMacroToken = 0;
+  unsigned EndMacroToken = 0;
+};

Please add brief doc comments to these variables. Having read the public API of 
this class, I still don't have an idea what these variables are.





Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:120
+  /// the original source file. The tranformation may not be possible if the
+  /// tokens cross macro invocations in the middle, e.g.
+  ///#define FOO 1*2

"The mapping fails if cross boundaries of macro expansions, that is, don't 
correspond to a complete top-level macro invocation"

Consider adding examples.

```
Given this source file:

#define FIRST f1 f2 f3
#define SECOND s1 s2 3

a FIRST b SECOND c  // expansion: a f1 f2 f3 b s1 s2 s3 c

toOffsetRange will map tokens like this:

input range => output range
a => a
s1 s2 s3 => SECOND
a f1 f2 f3 => a FIRST
a f1 => can't map
s1 s2 => s1 s2 within the macro definition
```

Could you add these to tests as well?



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:153
+  /// FIXME: we do not yet store tokens of directives, like #include, #define,
+  ///#pragma, etc.
+  llvm::ArrayRef macroTokens() const { return MacroTokens; }

... For the input above, macroTokens() should return {desired output}



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:191
+  /// i.e. after running Execute().
+  TokenBuffer consume() &&;
+

"Finalizes token collection"

Of course a function called "consume" consumes the result :)



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:191
+  /// i.e. after running Execute().
+  TokenBuffer consume() &&;
+

gribozavr wrote:
> "Finalizes token collection"
> 
> Of course a function called "consume" consumes the result :)
LLVM_NODISCARD?



Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:295
+MacroExpansion::tokens(const TokenBuffer &B) const {
+  return B.tokens().slice(BeginExpansionToken,
+  EndExpansionToken - BeginExpansionToken);

"ExpansionTokenBegin"?  "ExpansionTokenStartIndex"?



Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:367
+  if (FirstCall != Expansions.end() &&
+  (FirstCall->BeginExpansionToken < BeginIndex ||
+   EndIndex < FirstCall->EndExpansionToken)) {

`FirstCall->BeginExpansionToken != BeginIndex` ?



Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:368
+  (FirstCall->BeginExpansionToken < BeginIndex ||
+   EndIndex < FirstCall->EndExpansionToken)) {
+return llvm::None;

I don't understand why `EndIndex < FirstCall->EndExpansionToken` is needed -- 
isn't it redundant with the `LastCall` checks below?



Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:373
+  if (LastCall != Expansions.end() &&
+  (LastCall->BeginExpansionToken < BeginIndex ||
+   EndIndex < LastCall->EndExpansionToken)) {

`LastCall->BeginExpansionToken != BeginIndex`?



Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:374
+  (LastCall->BeginExpansionToken < BeginIndex ||
+   EndIndex < LastCall->EndExpansionToken)) {
+return llvm::None;

Also redundant with `FirstCall` checks?



Comment at: clang/unittests/Tooling/Syntax/TokenBufferTest.cpp:355
+  checkTokens("");
+  checkMacroInvocations({{"EMPTY", "", Code.range("m")},
+   {"EMPTY_FUNC(1+2+3)", "", Code.range("f")}});

`expectTokens`, `expectMacroInvocations`?



Comment at: clang/unittests/Tooling/Syntax/TokenBufferTest.cpp:376
+  // The parser eventually breaks the first '>>' into two tokens ('>' and '>'),
+  // but 

[PATCH] D60040: [clangd] Use capacity() instead of size() in RefSlab::bytes()

[Dmitri Gribenko via Phabricator via cfe-commits]

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

Do you have commit access?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60040



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

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Dmitri Gribenko via Phabricator via cfe-commits]

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

This fix makes sense to me, however, Richard Smith @rsmith is the best person 
to review this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60055



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

[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:756
   /// lazily.
   mutable llvm::DenseMap RedeclComments;
 

The name of this variable (and `RawCommentAndCacheFlags`) don't make sense 
after the change.



Comment at: clang/lib/AST/ASTContext.cpp:372
   D = adjustDeclToTemplate(D);
+  const Decl* CanonicalDecl = D->getCanonicalDecl();
 

jkorous wrote:
> arphaman wrote:
> > Why are we now checking for the canonical declaration instead of `D` as 
> > before?
> Before this we were caching comment for every redeclaration separately but 
> for every redeclaration the comment was the same.
> 
> As I understand it - for a given declaration we found the first comment in 
> the redeclaration chain and then saved it to the cache for every 
> redeclaration (the same comment).
> Later, during lookup, we iterated the redeclaration chain again and did a 
> lookup for every redeclaration (see L392 in the original implementation).
> 
> Unless I missed something, it's suboptimal in both memory consumption and 
> runtime overhead.
> 
> That's the reason why I want to cache the comment for the redeclaration group 
> as a whole. The only thing I am not entirely sure about is what key to use to 
> represent the whole redeclaration chain - maybe the first declaration in the 
> redeclaration chain would be better then the canonical declaration...
> 
> WDYT?
> As I understand it - for a given declaration we found the first comment in 
> the redeclaration chain and then saved it to the cache for every 
> redeclaration (the same comment).

Only if there was only one comment in the redeclaration chain.  If a 
redeclaration had a different comment, this function would return that comment.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60432



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

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

I'd be happy to land it, but I do want @rsmith to take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60055



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

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC358134: Check i < FD->getNumParams() before querying 
(authored by gribozavr, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60055?vs=194152&id=194581#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D60055

Files:
  lib/Sema/SemaTemplateInstantiate.cpp
  test/SemaCXX/PR41139.cpp
  test/SemaCXX/cxx1y-generic-lambdas.cpp


Index: lib/Sema/SemaTemplateInstantiate.cpp
===
--- lib/Sema/SemaTemplateInstantiate.cpp
+++ lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   unsigned i = PV->getFunctionScopeIndex();
   // This parameter might be from a freestanding function type within the
   // function and isn't necessarily referring to one of FD's parameters.
-  if (FD->getParamDecl(i) == PV)
+  if (i < FD->getNumParams() && FD->getParamDecl(i) == PV)
 return FD->getCanonicalDecl()->getParamDecl(i);
 }
   }
Index: test/SemaCXX/cxx1y-generic-lambdas.cpp
===
--- test/SemaCXX/cxx1y-generic-lambdas.cpp
+++ test/SemaCXX/cxx1y-generic-lambdas.cpp
@@ -944,6 +944,15 @@
   }(0)(0);
 }
 
+namespace PR41139 {
+  int y = [](auto outer) {
+return [](auto inner) {
+  using T = int(decltype(outer), decltype(inner));
+  return 0;
+};
+  }(0)(0);
+}
+
 namespace PR23716 {
 template
 auto f(T x) {
Index: test/SemaCXX/PR41139.cpp
===
--- test/SemaCXX/PR41139.cpp
+++ test/SemaCXX/PR41139.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+// This test should not crash.
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}


Index: lib/Sema/SemaTemplateInstantiate.cpp
===
--- lib/Sema/SemaTemplateInstantiate.cpp
+++ lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   unsigned i = PV->getFunctionScopeIndex();
   // This parameter might be from a freestanding function type within the
   // function and isn't necessarily referring to one of FD's parameters.
-  if (FD->getParamDecl(i) == PV)
+  if (i < FD->getNumParams() && FD->getParamDecl(i) == PV)
 return FD->getCanonicalDecl()->getParamDecl(i);
 }
   }
Index: test/SemaCXX/cxx1y-generic-lambdas.cpp
===
--- test/SemaCXX/cxx1y-generic-lambdas.cpp
+++ test/SemaCXX/cxx1y-generic-lambdas.cpp
@@ -944,6 +944,15 @@
   }(0)(0);
 }
 
+namespace PR41139 {
+  int y = [](auto outer) {
+return [](auto inner) {
+  using T = int(decltype(outer), decltype(inner));
+  return 0;
+};
+  }(0)(0);
+}
+
 namespace PR23716 {
 template
 auto f(T x) {
Index: test/SemaCXX/PR41139.cpp
===
--- test/SemaCXX/PR41139.cpp
+++ test/SemaCXX/PR41139.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+// This test should not crash.
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:728
 
-  class RawCommentAndCacheFlags {
+  class CommentAndOrigin {
   public:

`RawCommentAndOrigin`?



Comment at: clang/include/clang/AST/ASTContext.h:751
 
-  /// Mapping from declarations to comments attached to any
+  /// Mapping from canonical declarations to comments attached to any
   /// redeclaration.

Please remove "canonical", it is not correct according to the implementation.

Please also update the patch description that currently says "I assume we can 
cache comments for canonical declarations only, not for every redeclaration."



Comment at: clang/lib/AST/ASTContext.cpp:372
   D = adjustDeclToTemplate(D);
+  const Decl* CanonicalDecl = D->getCanonicalDecl();
 

jkorous wrote:
> gribozavr wrote:
> > jkorous wrote:
> > > arphaman wrote:
> > > > Why are we now checking for the canonical declaration instead of `D` as 
> > > > before?
> > > Before this we were caching comment for every redeclaration separately 
> > > but for every redeclaration the comment was the same.
> > > 
> > > As I understand it - for a given declaration we found the first comment 
> > > in the redeclaration chain and then saved it to the cache for every 
> > > redeclaration (the same comment).
> > > Later, during lookup, we iterated the redeclaration chain again and did a 
> > > lookup for every redeclaration (see L392 in the original implementation).
> > > 
> > > Unless I missed something, it's suboptimal in both memory consumption and 
> > > runtime overhead.
> > > 
> > > That's the reason why I want to cache the comment for the redeclaration 
> > > group as a whole. The only thing I am not entirely sure about is what key 
> > > to use to represent the whole redeclaration chain - maybe the first 
> > > declaration in the redeclaration chain would be better then the canonical 
> > > declaration...
> > > 
> > > WDYT?
> > > As I understand it - for a given declaration we found the first comment 
> > > in the redeclaration chain and then saved it to the cache for every 
> > > redeclaration (the same comment).
> > 
> > Only if there was only one comment in the redeclaration chain.  If a 
> > redeclaration had a different comment, this function would return that 
> > comment.
> I see. I made a wrong assumption - that for any two redeclarations the 
> redeclaration chain always starts from the same declaration.
I still don't think this patch is a no-op.

Here is the intended behavior:

```
/// First
void foo(); // (1), canonical decl

void foo(); // (2)

/// Third
void foo() {} // (3)

void foo(); // (4)
```

getRawCommentForAnyRedecl(1) => "First"
getRawCommentForAnyRedecl(2) => "First"
getRawCommentForAnyRedecl(3) => "Third"
getRawCommentForAnyRedecl(4) => "First"




Comment at: clang/lib/AST/ASTContext.cpp:373
 
-  // Check whether we have cached a comment for this declaration already.
-  {
-llvm::DenseMap::iterator Pos =
-RedeclComments.find(D);
+  auto CacheLookup = [&](const Decl *SpecificD) -> const RawComment * {
+llvm::DenseMap::iterator Pos =

`GetCommentFromCache`?



Comment at: clang/lib/AST/ASTContext.cpp:405
+  // Check whether we have cached a comment for this specific declaration.
+  if (auto * CachedComment = CacheLookup(D))
+return CachedComment;

No space after `*`.



Comment at: clang/lib/AST/ASTContext.cpp:415
+
+  // In case this is the canonical Decl we're done.
+  auto CanonicalD = D->getCanonicalDecl();

I don't understand the comment -- I read it as "if we got here, and the decl 
was canonical, and we haven't found anything already, we should return 
nullptr".  The code does something different.

Also, just to be clear, here is the intended behavior:

```
void foo(); // (1), canonical decl

/// Second
void foo(); // (2)
```

getRawCommentForAnyRedecl(1) => "Second"
getRawCommentForAnyRedecl(2) => "Second"




Comment at: clang/lib/AST/ASTContext.cpp:418
+  if (!CanonicalD)
+return nullptr;
 

When is it possible to not have a canonical decl?



Comment at: clang/lib/AST/ASTContext.cpp:421
+  // Check whether we have cached a comment for canonical declaration of this 
declaration.
+  if (auto * CachedComment = CacheLookup(CanonicalD))
+return CachedComment;

No space after `*`.



Comment at: clang/lib/AST/ASTContext.cpp:425
+  // We don't have comment for neither D, nor it's canonical declaration in 
cache and D doesn't have any comment attached to itself.
+  // Search for any comment attached to redeclarations of D and cache it for 
canonical declaration of D.
   for (auto I : D->redecls()) {

80 columns.


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

https://reviews.llvm.org/D60

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst:10
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+

IIRC it is possible to pass through compiler warnings through ClangTidy... WDYT 
about that instead of reimplementing the warning?

However we would lose the ability to "infer" `warn_unused_result` on functions 
that return `ERR_PTR`.  However, since the analysis is not 
cross-translation-unit, IDK how much value there is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963



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

[PATCH] D59963: [clang-tidy] Add a module for the Linux kernel.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst:10
+functions are marked with ``__attribute__((warn_unused_result))``, but
+the compiler warning for this attribute is not always enabled.
+

tmroeder wrote:
> gribozavr wrote:
> > IIRC it is possible to pass through compiler warnings through ClangTidy... 
> > WDYT about that instead of reimplementing the warning?
> > 
> > However we would lose the ability to "infer" `warn_unused_result` on 
> > functions that return `ERR_PTR`.  However, since the analysis is not 
> > cross-translation-unit, IDK how much value there is.
> I don't know exactly how to pass the warnings through, but I'd be interested 
> in learning how to do that. I agree that that would be cleaner than this 
> (partial) reimplementation.
> 
> Note that my code above does do something like that: I currently check that 
> the unused-result attribute is set on the return from the call.
> I don't know exactly how to pass the warnings through, but I'd be interested 
> in learning how to do that. I agree that that would be cleaner than this 
> (partial) reimplementation.

It seems like it is done by default, and `-Wunused-result` is enabled by 
default:

```
$ cat /tmp/example.cpp
int __attribute__((warn_unused_result)) foo();

void bar() {
  foo();
}
$ ./bin/clang-tidy /tmp/example.cpp
1 warning generated.
/tmp/example.cpp:4:3: warning: ignoring return value of function declared with 
'warn_unused_result' attribute [clang-diagnostic-unused-result]
  foo();
  ^
```

If it does not work for you, you can enable it on the command line:

```
clang-tidy -extra-arg=-Wunused-result -checks=clang-diagnostic-unused-result 
/tmp/example.cpp
```

> Note that my code above does do something like that: I currently check that 
> the unused-result attribute is set on the return from the call.

Yes, I was saying that we would lose the ability to do that.  However, is it 
that valuable?  The analysis in ClangTidy does not cross translation unit 
boundaries, so unless the caller and the callee are in the same file, this 
inference does not buy us much.

You could also use an existing check, `bugprone-unused-return-value`, without 
even requiring the function to be annotated with the attribute:

```
$ cat /tmp/example.cpp
int foo();

void bar() {
  foo();
}
$ ./bin/clang-tidy /tmp/example.cpp -config="{Checks: 
'bugprone-unused-return-value', CheckOptions: [{key: 
'bugprone-unused-return-value.CheckedFunctions', value: 'foo'}]}"
1 warning generated.
/tmp/example.cpp:4:3: warning: the value returned by this function should be 
used [bugprone-unused-return-value]
  foo();
  ^
/tmp/example.cpp:4:3: note: cast the expression to void to silence this warning
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59963



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Thanks for the checker!  We are hitting a crash on `_Atomic` fields, could you 
take a look?  https://bugs.llvm.org/show_bug.cgi?id=41590  Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58573



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

[PATCH] D61106: [analyzer][UninitializedObjectChecker] PR41590: Regard _Atomic types as primitive

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added a comment.

Thanks for the fix!




Comment at: test/Analysis/cxx-uninitialized-object.cpp:1145
+
+void entry() {
+  MyAtomicInt b;

`c11atomicTest()`


Repository:
  rC Clang

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

https://reviews.llvm.org/D61106



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

[PATCH] D61103: [clang] Add tryToAttachCommentsToDecls method to ASTContext

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> The FIXME in tests is fixed now.

... so instead of deleting the test, could you change it to show the current, 
better diagnostic?




Comment at: clang/include/clang/AST/ASTContext.h:818
+  /// For every comment not attached to any decl check if it should be attached
+  /// to any of \param Decls
+  ///

Please add a period.



Comment at: clang/include/clang/AST/ASTContext.h:818
+  /// For every comment not attached to any decl check if it should be attached
+  /// to any of \param Decls
+  ///

gribozavr wrote:
> Please add a period.
Please add a period.



Comment at: clang/lib/AST/ASTContext.cpp:494
+  // Explicitly not calling ExternalSource->ReadComments() as we're not
+  // interested in those.
+  ArrayRef RawComments = Comments.getComments();

Would be great to explain why (because we assume that the decls and their 
comments were parsed just now).  Otherwise the comment could enumerate a lot of 
other things that we are not calling here either...



Comment at: clang/lib/AST/ASTContext.cpp:564
+  for (const Decl *D : Decls) {
+D = adjustDeclToTemplate(D);
+if (!CanDeclHaveDocComment(D))

`getCommentForDecl` checks `D->isInvalidDecl()` first.



Comment at: clang/lib/AST/ASTContext.cpp:584
+// terminating early.
+for (auto CIt = RawComments.begin(); CIt != RawComments.end(); ++CIt) {
+  RawComment *C = *CIt;

Scanning all comments for every decl?  Isn't that O(n^2)?

Also logic duplication below.  I was expecting a call to 
`getRawCommentForDeclNoCache`, with an appropriate flag to disable loading 
external comments (it is a low-level API, users generally don't call it).


Repository:
  rC Clang

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

https://reviews.llvm.org/D61103



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

[PATCH] D61104: [clang][ASTContext] Try to avoid sorting comments for code completion

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:800
+  /// The result doesn't contain decls that don't have any comment attached.
+  std::unordered_map getRawCommentsForDeclsNoCache(
+  const std::unordered_map>

Why not DenseMap?



Comment at: clang/include/clang/AST/ASTContext.h:808
+  std::unordered_map
+  getRawCommentsForAnyRedecls(const std::vector &NDs) const;
+

Use ArrayRef.



Comment at: clang/lib/AST/ASTContext.cpp:303
+
+  return Result;
+}

I'm really worried about all the logic duplication here vs. existing code.



Comment at: clang/lib/AST/RawCommentList.cpp:366
+}
+  }
 }

Why is merging in `RawCommentList::addComment` not sufficient?



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3180
   if (IncludeBriefComments) {
+// Try to get the comment if it wasn't provided
+if (!Comment)

There are only a couple of callers of this function, can we change them all to 
provide a comment if it exists?



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3398
+  for (const auto *const ND : NDs) {
+if (const ObjCMethodDecl *M = dyn_cast(ND)) {
+  if (const ObjCPropertyDecl *PDecl = M->findPropertyDecl()) {

This method decl logic looks out of place here.  It should be pushed down into 
the core logic for attaching comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61104



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Thank you for the fix!  Another crash: 
https://bugs.llvm.org/show_bug.cgi?id=41611


Repository:
  rC Clang

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

https://reviews.llvm.org/D58573



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

[PATCH] D61246: [analyzer][UninitializedObjectChecker] PR41611: Regard vector types as primitive

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Thank you for the quick fix!




Comment at: test/Analysis/cxx-uninitialized-object.cpp:1155
+void __vector_size__LongTest() {
+  VectorSizeLong v;
+}

Could you also add tests where the vector member is being subscripted?  for 
example, `v.x[0] = 0;`



Comment at: test/Analysis/cxx-uninitialized-object.cpp:1155
+void __vector_size__LongTest() {
+  VectorSizeLong v;
+}

gribozavr wrote:
> Could you also add tests where the vector member is being subscripted?  for 
> example, `v.x[0] = 0;`
Should there be a warning about the member being uninitialized?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61246



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

[PATCH] D61246: [analyzer][UninitializedObjectChecker] PR41611: Regard vector types as primitive

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: test/Analysis/cxx-uninitialized-object.cpp:1155
+void __vector_size__LongTest() {
+  VectorSizeLong v;
+}

gribozavr wrote:
> gribozavr wrote:
> > Could you also add tests where the vector member is being subscripted?  for 
> > example, `v.x[0] = 0;`
> Should there be a warning about the member being uninitialized?
Just to be clear -- if there should be a warning, I don't think this patch has 
to necessarily fix everything in the checker and static analyzer core to 
support vector types, the fix for the crash is valuable by itself.  However, if 
there should be a warning, this test should have a fixme.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61246



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

[PATCH] D61103: [clang] Add tryToAttachCommentsToDecls method to ASTContext

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> Also, IIUC the test case that I deleted wasn't actually supposed to produce 
> any diagnostics and the fact that it did was a bug. We could keep it as a 
> regression test but I think it has a rather low value. WDYT?

What do you mean?  The issue that the test is trying to show is that a single 
//-line in a ///-comment breaks the doc comment and produces weird errors.  
Instead it should tell the user that it looks like there's an unintended 
//-line.


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

https://reviews.llvm.org/D61103



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

[PATCH] D61103: [clang] Add tryToAttachCommentsToDecls method to ASTContext

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:584
+// terminating early.
+for (auto CIt = RawComments.begin(); CIt != RawComments.end(); ++CIt) {
+  RawComment *C = *CIt;

jkorous wrote:
> gribozavr wrote:
> > Scanning all comments for every decl?  Isn't that O(n^2)?
> > 
> > Also logic duplication below.  I was expecting a call to 
> > `getRawCommentForDeclNoCache`, with an appropriate flag to disable loading 
> > external comments (it is a low-level API, users generally don't call it).
> The important thing is that the expensive operation is source location 
> decomposition. That's why I cache everything - `GetCachedCommentBegin` etc.
> 
> So while you're right that iteration-wise it's O(iteration*c*d) it`s actually 
> O(decompose*c + decompose*d) because of the caching. The current code (which 
> is sorting all the comments) is at least O(decompose*c*ln(c)) once you have 
> more comments than `sqrt(300)` (==`MagicCacheSize` in 
> `SourceManager::getInBeforeInTUCache()`).
> 
> That being said - you're right that just not-loading external comments in 
> `getRawCommentForDeclNoCache` definitely has it's appeal. I'm running a test 
> now get some idea about performance of both approaches.
> 
> BTW in theory we could also do one of these:
> 1. Allow clients to transparently set `MagicCacheSize` in 
> `SourceManager::getInBeforeInTUCache()` which is used for SourceLocation 
> sorting (`BeforeThanCompare`) is currently hard-coded to 300 
> while we are comparing ~100k x ~100k locations.
> 2. Change caching strategies in `SourceManager::getFileID` and 
> `SourceManager::getLineNumber`.
> So while you're right that iteration-wise it's O(iteration*c*d) it`s actually 
> O(decompose*c + decompose*d) because of the caching. 

The cost of decomposing is non-trivial, but the cost of each iteration is still 
at least a hash table lookup.

> That being said - you're right that just not-loading external comments in 
> getRawCommentForDeclNoCache definitely has it's appeal. I'm running a test 
> now get some idea about performance of both approaches.

Reopening, waiting for the results.


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

https://reviews.llvm.org/D61103



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