This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Parse std::make_unique, and emit template diagnostics at expansion.
ClosedPublic

Authored by sammccall on Jun 7 2020, 3:09 PM.

Details

Summary

Parsing std::make_unique is an exception to the usual non-parsing of function
bodies in the preamble. (A hook is added to PreambleCallbacks to allow this).
This allows us to diagnose make_unique<Foo>(wrong arg list), and opens the door
to providing signature help (by detecting where the arg list is forwarded to).
This function is trivial (checked libc++ and libstdc++) and doesn't result in
any extra templates being instantiated, so this should be cheap.

This uncovered a second issue (already visible with class templates)...

Errors produced by template instantiation have primary locations within the
template, with instantiation stack reported as notes.
For templates defined in headers, these end up reported at the #include
directive, which isn't terribly helpful as the header itself is probably fine.
This patch reports them at the instantiation site (the first location in the
instantiation stack that's in the main file). This in turn required a bit of
refactoring in Diagnostics so we can delay relocating the diagnostic until all
notes are available.

Diff Detail

Event Timeline

sammccall created this revision.Jun 7 2020, 3:09 PM
hokein accepted this revision.Jun 9 2020, 1:16 AM
hokein added inline comments.
clang-tools-extra/clangd/Diagnostics.cpp
694

nit: we have a same method call on Line 568, I think we can use a separate local variable to store the result at the beginning of this function.

738

shall we also reset LastDiagLoc?

clang-tools-extra/clangd/Diagnostics.h
151

nit: document what do pair.first and pair.second represent

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
282

nit: an extra ; at the end.

clang/include/clang/Frontend/PrecompiledPreamble.h
297

nit: documentation.

This revision is now accepted and ready to land.Jun 9 2020, 1:16 AM
sammccall marked 6 inline comments as done.Jun 9 2020, 3:21 AM
sammccall added inline comments.
clang-tools-extra/clangd/Diagnostics.cpp
738

We can, but it's redundant (dead store) and seems a bit "loose".
The LastDiag* variables are only meaningful if LastDiag is set.
Added comments to LastDiagLoc and LastDiagOriginallyError for this.

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
282

(Rather extra ", right?)

sammccall updated this revision to Diff 269479.Jun 9 2020, 3:21 AM
sammccall marked 2 inline comments as done.

review comments

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 9 2020, 6:05 AM

Breaks tests on windows: http://45.33.8.238/win/17229/step_9.txt

Please take a look and revert if it takes a while to fix. (Might just be the usual delayed template parsing thing.)

Breaks tests on windows: http://45.33.8.238/win/17229/step_9.txt

Please take a look and revert if it takes a while to fix. (Might just be the usual delayed template parsing thing.)

It's a new and exciting delayed template parsing thing! Need D81474 before this can be landed.