This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add a missing include-fixer test for incomplete_type, NFC.
ClosedPublic

Authored by hokein on Oct 7 2020, 6:30 AM.

Details

Summary

Also sort the list to make it easier to verify with the implementation
code.

Diff Detail

Event Timeline

hokein created this revision.Oct 7 2020, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2020, 6:30 AM
hokein requested review of this revision.Oct 7 2020, 6:30 AM
kadircet added inline comments.Oct 7 2020, 7:04 AM
clang-tools-extra/clangd/IncludeFixer.cpp
71

nit: maybe revert this change, or put err_incomplete_type to the last position to make the list alphabetically ordered.

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

should this be $type[[a]] ?

711

i don't follow what we gain by this change?

715

why do we need c++17 ?

738

can you move this back to original position ?

hokein added inline comments.Oct 7 2020, 11:05 AM
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
709

yeah, but this is the actual diagnostic range, though it is wired.

715

The newly-added test case is using C++17 feature (structure binding), needs this flag to suppress a diagnostic warning.

738

yeah, I reordered the list.

there are two orders: the one listed here, and the one listed in IncludeFixer.cpp.

The motivation is to make them aligned, so that it would be easier to compare and spot the difference. The current state is not friendly to readers.

hokein updated this revision to Diff 296767.Oct 7 2020, 12:10 PM

update: don't change the order in test.

kadircet accepted this revision.Oct 8 2020, 3:01 AM

still LGTM

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

ah okay i see it now, structured binding wasn't obvious from the test case above (i thought there was a typo :D

738

i don't think the orderings in the source vs test is that relevant, but SGTM if it helps you. mainly because:

  • as it might be harder in future to trigger diagnostics in the order we want
  • it is an invariant hard to maintain without at least the reviewer or author of changes knowing about(and remembering) it
This revision is now accepted and ready to land.Oct 8 2020, 3:01 AM
hokein added inline comments.Oct 8 2020, 4:28 AM
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
738

yeah, it does help me, especially when I start adding more diagnostics.