Page MenuHomePhabricator

WIP: Fix Diagnostic layering, moving diagnostics out of Basic
Needs ReviewPublic

Authored by dblaikie on Dec 18 2017, 9:00 AM.

Details

Reviewers
rsmith
Summary

This goes part-way down the path of moving the actual diagnostics out of Clang's Basic library into the respective libraries that use those diagnostics. The end goal would be that a client program calling into those libraries would be responsible for building the necessary table containing all diagnostics from all components they're calling into (so a clang refactoring tool wouldn't need to pull in the codegen diagnostics, for example).

But I ran out of momentum a bit & looking for some sanity checking/help/encouragement/direction.

The basic idea has been to make the static APIs of DiagnosticIDs non-static, then give DiagnosticIDs a ctor that takes the table of diagnostics - as an intermediate step, I've left the no-arg ctor in, and dabbled with making it a bool ctor just as a way to find the places that still use it and clean them up while leaving a handful of correct uses of DiagnosticIDs construction to flesh out (build the table from the desired diagnostics, etc) later.

Partly wondering if this is the right general direction, if some of these changes are worth committing incrementally as I plumb through DiagnosticIDs objects through more APIs.

I remember you, Richard, pointing out that having a generic way to build the diagnostics table based on each different clients needs of different subsets of all diagnostics wasn't going to be pretty owing to the inability to use the preprocessor to conditionally/dynamically #include things in different contexts (I'm sure I'm doing a bad job of explaining that - but I'm understanding what you meant, that there wouldn't be a quick few lines of "define diagnostics table containing these 3 subcomponents" because you'd have to include those 3 subcomponents diagnostic tables in several different contexts to build up several different data structures/subtables/etc... ). & maybe I need to dig into that more, look at how that could end up, see if it's reasonable/good before doing much more here, rather than putting that off until the end.

Diff Detail

Event Timeline

dblaikie created this revision.Dec 18 2017, 9:00 AM
rsmith added a comment.Jan 4 2018, 2:17 PM

Yes, I think this is the right general direction -- in general, moving towards making clang's diagnostic infrastructure a reusable component that isn't tied to a particular diagnostics table, and in particular moving the knowledge of the complete set of diagnostics and diagnostic groups out of the diagnostics infrastructure and into the frontend tool.

tools/driver/cc1_main.cpp
172–262

The best home for this is probably the FrontendTool library.

craig.topper added inline comments.
lib/Basic/DiagnosticIDs.cpp
27–28

This comment is out of date with the struct being renamed.

73

Did this static_assert stuff get lost?

tools/driver/cc1_main.cpp
211

StaticDiagInfoRec was renamed.

dblaikie updated this revision to Diff 138238.Mar 13 2018, 11:45 AM

Mostly done (functionally, various tidying up possible)

dblaikie updated this revision to Diff 138240.Mar 13 2018, 11:46 AM

One possible solution - skipping BasicOnly & having everything depend on FrontendTool.

dblaikie updated this revision to Diff 138243.Mar 13 2018, 11:58 AM

Address Craig Topper's feedback (StaticDiagInfoRec -> DiagInfoRec and VALIDATE_DIAG_SIZE)

dblaikie updated this revision to Diff 138255.Mar 13 2018, 1:49 PM

Actually move AllDiagnostics.h from Basic to FrontendTool and deal with the fallout for that.

Updated with a functional implementation - a few points:

  • Some tests could use only the Common diagnostics (see one of the intermediate changes where I moved those tests over to use all diagnostics) except for the tablegen required for the diagnostic groups makes that a bit heavier - though doing so would help reduce dependencies.
  • Naming (of the "DiagnosticTable" header, and the functions/entities contained there) could be improved - open to ideas
  • API (currently passing in like 6 different raw arrays and ArrayRefs to DiagnosticIDs) could be improved - open to ideas
dblaikie updated this revision to Diff 142031.Apr 11 2018, 9:58 AM

Finish functional prototype - this now does remove the dependency from Basic to the various other libraries.

There are still constants defining the range of each category of diagnostics that remain in Basic - not sure those can be removed, though.

I've pushed this further - it builds now & has no fallback in libBasic to the old behavior/table.

One remaining thing I'm unsure about is the table of DIAG_START_* values. Do you think this is OK/good to leave hardcoded in diagnostics? Should I somehow remove the need for that global knowledge (dynamically allocating the start ranges of each grouping?)? Or go the other way & decide this global information renders this direction questionable and just sink all the diagnostics into Basic explicitly/properly?