dblaikie added a comment.
Sorry if this is derailing, but I wonder/worry about a few things here:
1. Compounding structured output with phraseology - it seems like it might be
worthwhile for these to be separate issues (doesn't mean the terminal output
has to say exactly the same thing - as you've mentioned, we could have some
fields that aren't rendered in some modes (eg: maybe a terminal only gets the
summary, and not the reason - or perhaps you can ask for reasons to be provided
if you don't mind the verbosity)). In the example case include here -
describing the template parameter kind mismatch seems OK for Clang's current
textual diagnostic experience & I don't think that would need to be limited to
only the SARIF output?
2. Could the new phraseology be included in a separate/parallel diagnostic
file, essentially akin to a translation? Clang never did get support for
multiple languages in its diagnostics, but maybe this is the time to do that
work? And then have this new phrasing as the first "translation" of sorts?
3. I'm not sure I'd commit to calling the current diagnostic experience
"legacy" just yet & maybe this is my broadest feedback: It'd be great to avoid
a two-system situation with the intent to merge things back in later.
I think what I'd like to see (though I'm far from the decider in any of this)
would be that the existing underlying structured diagnostics system could have
a new emission system, that produces SARIF instead of text on a terminal - same
underlying structured representation, but providing that more directly (in
SARIF/JSON/etc) so that it's not lossy in the conversion to an output format -
essentially a machine-readable mode for clang's diagnostic output. (to a file,
to the terminal, beside the existing terminal output or instead of it, whatever
folks prefer) - this would, ideally, initially require no changes to diagnostic
API usage across clang.
Diagnostic quality improvements, like the template parameter kind mismatch can
be committed independently as desired.
Diagnostic infrastructure could be improved/expanded - adding the "reason"
field, and a way to emit that (perhaps only in SARIF output, but maybe there's
room for a flag to emit it in terminal diagnostics too).
Multiple diagnostic text files supported, and the ability to choose which
diagnostic text - initially with the intent to support this newly proposed
phrasing approach, but equally could be used for whole language translation.
Pretty much all of these features could be implemented somewhat in parallel,
each feature would be of value independently to varying degrees, and we
wouldn't end up with a deprecated/legacy/not ready yet situation (the only "not
ready yet" part might be adding the new diagnostic phrasings - perhaps
initially there'd be some way to fallback to the traditional diagnostics, so
that the whole database didn't need to be translated wholesale - and once it's
all translated and there's a lot of user feedback on the direction, we could
consider changing the diagnostic database default, perhaps eventually
deprecating the old database, and eventually removing it - without major API
upheaval/renaming/addition/removal)
================
Comment at: clang/lib/Basic/DiagnosticIDs.cpp:524
+
+ // FIXME(spaceship): default when C++20 becomes the default
+ bool operator==(const DiagDesc &Other) const {
----------------
I don't think LLVM generally uses `(xxx)` in FIXMEs, or if we do it's probably
only for bug numbers and maybe developer user names (though the latter's not
ideal - people come and go, bug numbers are forever (kinda)) - not sure what
the "spaceship" in there is meant to communicate (I'm aware of the C++20
spaceship operator, but I'd expect the thing in the `()` to be some list of
known entities that we keep track of somewhere?)
Maybe `// FIXME: Use spaceship operator in C++20`?
================
Comment at: clang/lib/Basic/DiagnosticIDs.cpp:812-825
namespace {
- struct WarningOption {
- uint16_t NameOffset;
- uint16_t Members;
- uint16_t SubGroups;
- StringRef Documentation;
-
- // String is stored with a pascal-style length byte.
- StringRef getName() const {
- return StringRef(DiagGroupNames + NameOffset + 1,
- DiagGroupNames[NameOffset]);
- }
- };
-}
+struct WarningOption {
+ uint16_t NameOffset;
+ uint16_t Members;
+ uint16_t SubGroups;
+ StringRef Documentation;
+
----------------
Unrelated/accidental reformatting? (please commit separately if it's being
committed)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138939/new/
https://reviews.llvm.org/D138939
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
