Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move documentation about -verify from a header to public docs #73694

Merged

Conversation

AaronBallman
Copy link
Collaborator

The internals manual seems like a more obvious home for the details instead of hiding them away in a header file and relying on doxygen output to document it publicly.

The internals manual seems like a more obvious home for the details
instead of hiding them away in a header file and relying on doxygen
output to document it publicly.
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Nov 28, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 28, 2023

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

The internals manual seems like a more obvious home for the details instead of hiding them away in a header file and relying on doxygen output to document it publicly.


Full diff: https://github.com/llvm/llvm-project/pull/73694.diff

2 Files Affected:

  • (modified) clang/docs/InternalsManual.rst (+167)
  • (modified) clang/include/clang/Frontend/VerifyDiagnosticConsumer.h (+2-150)
diff --git a/clang/docs/InternalsManual.rst b/clang/docs/InternalsManual.rst
index b7d88d3d67d0a0c..f8e3da5f9736829 100644
--- a/clang/docs/InternalsManual.rst
+++ b/clang/docs/InternalsManual.rst
@@ -3309,6 +3309,173 @@ are similar.
      as syntax highlighting, cross-referencing, and so on.  The
      ``c-index-test`` helper program can be used to test these features.
 
+Testing
+-------
+All functional changes to Clang should come with test coverage demonstrating
+the change in behavior.
+
+Verifying Diagnostics
+^^^^^^^^^^^^^^^^^^^^^
+Clang ``-cc1`` supports the ``-verify`` command line option as a way to
+validate diagnostic behavior. This option will use special comments within the
+test file to verify that expected diagnostics appear in the correct source
+locations. If all of the expected diagnostics match the actual output of Clang,
+then the invocation will return normally. If there are discrepancies between
+the expected and actual output, Clang will emit detailed information about
+which expected diagnostics were not seen or which unexpected diagnostics were
+seen, etc. A complete example is:
+
+.. code-block: c++
+
+  // RUN: %clang_cc1 -verify %s
+  int A = B; // expected-error {{use of undeclared identifier 'B'}}
+
+If the test is run and the expected error is emitted on the expected line, the
+diagnostic verifier will pass. However, if the expected error does not appear
+or appears in a different location than expected, or if additional diagnostics
+appear, the diagnostic verifier will fail and emit information as to why.
+
+The ``-verify`` command optionally accepts a comma-delimited list of one or
+more verification prefixes that can be used to craft those special comments.
+Each prefix must start with a letter and contain only alphanumeric characters,
+hyphens, and underscores. ``-verify`` by itself is equivalent to
+``-verify=expected``, meaning that special comments will start with
+``expected``. Using different prefixes makes it easier to have separate
+``RUN:`` lines in the same test file which result in differing diagnostic
+behavior. For example:
+
+.. code-block:: c++
+
+  // RUN: %clang_cc1 -verify=foo,bar %s
+
+  int A = B; // foo-error {{use of undeclared identifier 'B'}}
+  int C = D; // bar-error {{use of undeclared identifier 'D'}}
+  int E = F; // expected-error {{use of undeclared identifier 'F'}}
+
+The verifier will recognize ``foo-error`` and ``bar-error`` as special comments
+but will not recognize ``expected-error`` as one because the ``-verify`` line
+does not contain that as a prefix. Thus, this test would fail verification
+because an unexpected diagnostic would appear on the declaration of ``E``.
+
+Multiple occurrences accumulate prefixes.  For example,
+``-verify -verify=foo,bar -verify=baz`` is equivalent to
+``-verify=expected,foo,bar,baz``.
+
+Specifying Diagnostics
+^^^^^^^^^^^^^^^^^^^^^^
+Indicating that a line expects an error or a warning is simple. Put a comment
+on the line that has the diagnostic, use
+``expected-{error,warning,remark,note}`` to tag if it's an expected error,
+warning, remark, or note (respectively), and place the expected text between
+``{{`` and ``}}`` markers. The full text doesn't have to be included, only
+enough to ensure that the correct diagnostic was emitted. (Note: full text
+should be included in test cases unless there is a compelling reason to use
+truncated text instead.)
+
+Here's an example of the most commonly used way to specify expected
+diagnostics:
+
+.. code-block: c++
+
+  int A = B; // expected-error {{use of undeclared identifier 'B'}}
+
+You can place as many diagnostics on one line as you wish. To make the code
+more readable, you can use slash-newline to separate out the diagnostics.
+
+Alternatively, it is possible to specify the line on which the diagnostic
+should appear by appending ``@<line>`` to ``expected-<type>``, for example:
+
+.. code-block: c++
+
+  #warning some text
+  // expected-warning@10 {{some text}}
+
+The line number may be absolute (as above), or relative to the current line by
+prefixing the number with either ``+`` or ``-``.
+
+If the diagnostic is generated in a separate file, for example in a shared
+header file, it may be beneficial to be able to declare the file in which the
+diagnostic will appear, rather than placing the ``expected-*`` directive in the
+actual file itself. This can be done using the following syntax:
+
+.. code-block: c++
+
+  // expected-error@path/include.h:15 {{error message}}
+
+The path can be absolute or relative and the same search paths will be used as
+for ``#include`` directives. The line number in an external file may be
+substituted with ``*`` meaning that any line number will match (useful where
+the included file is, for example, a system header where the actual line number
+may change and is not critical).
+
+As an alternative to specifying a fixed line number, the location of a
+diagnostic can instead be indicated by a marker of the form ``#<marker>``.
+Markers are specified by including them in a comment, and then referenced by
+appending the marker to the diagnostic with ``@#<marker>``, as with:
+
+.. code-block: c++
+
+  #warning some text  // #1
+  // ... other code ...
+  // expected-warning@#1 {{some text}}
+
+The name of a marker used in a directive must be unique within the compilation.
+
+The simple syntax above allows each specification to match exactly one
+diagnostic. You can use the extended syntax to customize this. The extended
+syntax is ``expected-<type> <n> {{diag text}}``, where ``<type>`` is one of
+``error``, ``warning``, ``remark``, or ``note``, and ``<n>`` is a positive
+integer. This allows the diagnostic to appear as many times as specified. For
+example:
+
+.. code-block: c++
+
+  void f(); // expected-note 2 {{previous declaration is here}}
+
+Where the diagnostic is expected to occur a minimum number of times, this can
+be specified by appending a ``+`` to the number. For example:
+
+.. code-block: c++
+
+  void f(); // expected-note 0+ {{previous declaration is here}}
+  void g(); // expected-note 1+ {{previous declaration is here}}
+
+In the first example, the diagnostic becomes optional, i.e. it will be
+swallowed if it occurs, but will not generate an error if it does not occur. In
+the second example, the diagnostic must occur at least once. As a short-hand,
+"one or more" can be specified simply by ``+``. For example:
+
+.. code-block: c++
+
+  void g(); // expected-note + {{previous declaration is here}}
+
+A range can also be specified by ``<n>-<m>``. For example:
+
+.. code-block: c++
+
+  void f(); // expected-note 0-1 {{previous declaration is here}}
+
+In this example, the diagnostic may appear only once, if at all.
+
+Regex matching mode may be selected by appending ``-re`` to the diagnostic type
+and including regexes wrapped in double curly braces in the directive, such as:
+
+.. code-block: c++
+
+  expected-error-re {{format specifies type 'wchar_t **' (aka '{{.+}}')}}
+
+Examples matching error: "variable has incomplete type 'struct s'"
+
+.. code-block: c++
+
+  // expected-error {{variable has incomplete type 'struct s'}}
+  // expected-error {{variable has incomplete type}}
+
+  // expected-error-re {{variable has type 'struct {{.}}'}}
+  // expected-error-re {{variable has type 'struct {{.*}}'}}
+  // expected-error-re {{variable has type 'struct {{(.*)}}'}}
+  // expected-error-re {{variable has type 'struct{{[[:space:]](.*)}}'}}
+
 Feature Test Macros
 ===================
 Clang implements several ways to test whether a feature is supported or not.
diff --git a/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h b/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
index 500a7e11ab9ac2a..ddfae2666c4c397 100644
--- a/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
+++ b/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
@@ -32,156 +32,8 @@ class TextDiagnosticBuffer;
 
 /// VerifyDiagnosticConsumer - Create a diagnostic client which will use
 /// markers in the input source to check that all the emitted diagnostics match
-/// those expected.
-///
-/// INVOKING THE DIAGNOSTIC CHECKER:
-///
-/// VerifyDiagnosticConsumer is typically invoked via the "-verify" option to
-/// "clang -cc1".  "-verify" is equivalent to "-verify=expected", so all
-/// diagnostics are typically specified with the prefix "expected".  For
-/// example:
-///
-/// \code
-///   int A = B; // expected-error {{use of undeclared identifier 'B'}}
-/// \endcode
-///
-/// Custom prefixes can be specified as a comma-separated sequence.  Each
-/// prefix must start with a letter and contain only alphanumeric characters,
-/// hyphens, and underscores.  For example, given just "-verify=foo,bar",
-/// the above diagnostic would be ignored, but the following diagnostics would
-/// be recognized:
-///
-/// \code
-///   int A = B; // foo-error {{use of undeclared identifier 'B'}}
-///   int C = D; // bar-error {{use of undeclared identifier 'D'}}
-/// \endcode
-///
-/// Multiple occurrences accumulate prefixes.  For example,
-/// "-verify -verify=foo,bar -verify=baz" is equivalent to
-/// "-verify=expected,foo,bar,baz".
-///
-/// SPECIFYING DIAGNOSTICS:
-///
-/// Indicating that a line expects an error or a warning is simple. Put a
-/// comment on the line that has the diagnostic, use:
-///
-/// \code
-///   expected-{error,warning,remark,note}
-/// \endcode
-///
-/// to tag if it's an expected error, remark or warning, and place the expected
-/// text between {{ and }} markers. The full text doesn't have to be included,
-/// only enough to ensure that the correct diagnostic was emitted.
-///
-/// Here's an example:
-///
-/// \code
-///   int A = B; // expected-error {{use of undeclared identifier 'B'}}
-/// \endcode
-///
-/// You can place as many diagnostics on one line as you wish. To make the code
-/// more readable, you can use slash-newline to separate out the diagnostics.
-///
-/// Alternatively, it is possible to specify the line on which the diagnostic
-/// should appear by appending "@<line>" to "expected-<type>", for example:
-///
-/// \code
-///   #warning some text
-///   // expected-warning@10 {{some text}}
-/// \endcode
-///
-/// The line number may be absolute (as above), or relative to the current
-/// line by prefixing the number with either '+' or '-'.
-///
-/// If the diagnostic is generated in a separate file, for example in a shared
-/// header file, it may be beneficial to be able to declare the file in which
-/// the diagnostic will appear, rather than placing the expected-* directive in
-/// the actual file itself.  This can be done using the following syntax:
-///
-/// \code
-///   // expected-error@path/include.h:15 {{error message}}
-/// \endcode
-///
-/// The path can be absolute or relative and the same search paths will be used
-/// as for #include directives.  The line number in an external file may be
-/// substituted with '*' meaning that any line number will match (useful where
-/// the included file is, for example, a system header where the actual line
-/// number may change and is not critical).
-///
-/// As an alternative to specifying a fixed line number, the location of a
-/// diagnostic can instead be indicated by a marker of the form "#<marker>".
-/// Markers are specified by including them in a comment, and then referenced
-/// by appending the marker to the diagnostic with "@#<marker>":
-///
-/// \code
-///   #warning some text  // #1
-///   // expected-warning@#1 {{some text}}
-/// \endcode
-///
-/// The name of a marker used in a directive must be unique within the
-/// compilation.
-///
-/// The simple syntax above allows each specification to match exactly one
-/// error.  You can use the extended syntax to customize this. The extended
-/// syntax is "expected-<type> <n> {{diag text}}", where \<type> is one of
-/// "error", "warning" or "note", and \<n> is a positive integer. This allows
-/// the diagnostic to appear as many times as specified. Example:
-///
-/// \code
-///   void f(); // expected-note 2 {{previous declaration is here}}
-/// \endcode
-///
-/// Where the diagnostic is expected to occur a minimum number of times, this
-/// can be specified by appending a '+' to the number. Example:
-///
-/// \code
-///   void f(); // expected-note 0+ {{previous declaration is here}}
-///   void g(); // expected-note 1+ {{previous declaration is here}}
-/// \endcode
-///
-/// In the first example, the diagnostic becomes optional, i.e. it will be
-/// swallowed if it occurs, but will not generate an error if it does not
-/// occur.  In the second example, the diagnostic must occur at least once.
-/// As a short-hand, "one or more" can be specified simply by '+'. Example:
-///
-/// \code
-///   void g(); // expected-note + {{previous declaration is here}}
-/// \endcode
-///
-/// A range can also be specified by "<n>-<m>".  Example:
-///
-/// \code
-///   void f(); // expected-note 0-1 {{previous declaration is here}}
-/// \endcode
-///
-/// In this example, the diagnostic may appear only once, if at all.
-///
-/// Regex matching mode may be selected by appending '-re' to type and
-/// including regexes wrapped in double curly braces in the directive, such as:
-///
-/// \code
-///   expected-error-re {{format specifies type 'wchar_t **' (aka '{{.+}}')}}
-/// \endcode
-///
-/// Examples matching error: "variable has incomplete type 'struct s'"
-///
-/// \code
-///   // expected-error {{variable has incomplete type 'struct s'}}
-///   // expected-error {{variable has incomplete type}}
-///
-///   // expected-error-re {{variable has type 'struct {{.}}'}}
-///   // expected-error-re {{variable has type 'struct {{.*}}'}}
-///   // expected-error-re {{variable has type 'struct {{(.*)}}'}}
-///   // expected-error-re {{variable has type 'struct{{[[:space:]](.*)}}'}}
-/// \endcode
-///
-/// VerifyDiagnosticConsumer expects at least one expected-* directive to
-/// be found inside the source code.  If no diagnostics are expected the
-/// following directive can be used to indicate this:
-///
-/// \code
-///   // expected-no-diagnostics
-/// \endcode
+/// those expected. See clang/docs/InternalsManual.rst for details about how to
+/// write tests to verify diagnostics.
 ///
 class VerifyDiagnosticConsumer: public DiagnosticConsumer,
                                 public CommentHandler {

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urgh, I finally set a bookmark on that page, so this is going to break that!

But yeah, this makes a lot more sense here, and I think ends up being easier to maintain. It passes a quick proofread, so lgtm.

@philnik777
Copy link
Contributor

Would it make sense to document this more publicly? While it's designed to be an internal tool, it's really useful for people who want to make sure their library produces high quality diagnostics (e.g. nodiscard, static_asserts etc.). I'm sure you are aware that libc++ uses -verify too.

@Endilll
Copy link
Contributor

Endilll commented Nov 29, 2023

Would it make sense to document this more publicly? While it's designed to be an internal tool, it's really useful for people who want to make sure their library produces high quality diagnostics (e.g. nodiscard, static_asserts etc.). I'm sure you are aware that libc++ uses -verify too.

While I'm sympathetic to the intent, making this more public bring up the question what are we going to promise the users. I believe status quo is that we can do whatever we feel like to, coordinating it with libc++ people, which sit at a proverbial next desk to us. So I'm not opposed to this, but I'd like to see how we going to address this concern.

@AaronBallman
Copy link
Collaborator Author

AaronBallman commented Nov 29, 2023

Would it make sense to document this more publicly? While it's designed to be an internal tool, it's really useful for people who want to make sure their library produces high quality diagnostics (e.g. nodiscard, static_asserts etc.). I'm sure you are aware that libc++ uses -verify too.

Because -verify is a -cc1 option and not a driver-level option, it's not really user-facing despite being usable (and used!) outside of Clang. So I think the internals manual is the most appropriate place for the details; it's kind of akin to an internal-use pragma or intrinsic. (As @Endilll says, it's something we want to retain the ability to modify the behavior of at a whim, subject to usual coordination between LLVM community projects.)

@AaronBallman AaronBallman merged commit 15798f4 into llvm:main Nov 29, 2023
6 checks passed
@AaronBallman AaronBallman deleted the aballman-diagnostic-verifier-docs branch November 29, 2023 12:56
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Nov 30, 2023
Local branch amd-gfx dd44063 Merged main:0fac9da7342e into amd-gfx:8e59270d9d71
Remote branch main 15798f4 Move documentation about -verify from a header to public docs (llvm#73694)
@Endilll
Copy link
Contributor

Endilll commented Dec 6, 2023

@AaronBallman After moving the docs, none of the code blocks are rendering:
https://clang.llvm.org/docs/InternalsManual.html#verifying-diagnostics

@Endilll
Copy link
Contributor

Endilll commented Dec 6, 2023

It would also be nice to fix the link to full -verify docs in https://clang.llvm.org/docs/InternalsManual.html#the-diagnosticconsumer-interface

@AaronBallman
Copy link
Collaborator Author

@AaronBallman After moving the docs, none of the code blocks are rendering: https://clang.llvm.org/docs/InternalsManual.html#verifying-diagnostics

huh? there are zero sphinx diagnostics about the blocks, so that's exciting.

@AaronBallman
Copy link
Collaborator Author

Neato! .. code-block: c++ should be: .. code-block:: c++ -- did someone disable sphinx diagnostics or something? That should have been diagnosed...

@AaronBallman
Copy link
Collaborator Author

It would also be nice to fix the link to full -verify docs in https://clang.llvm.org/docs/InternalsManual.html#the-diagnosticconsumer-interface

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants