This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Include the diagnostics's code when comparing diagnostics
ClosedPublic

Authored by nridge on Jun 13 2019, 10:53 PM.

Event Timeline

nridge created this revision.Jun 13 2019, 10:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 10:53 PM

Could you also add a lit test for the case you mentioned in the github issue?

You can find pointers in clang-tools-extra/clangd/test/diagnostics.test and clang-tools-extra/clangd/test/fixits-codeaction.test

clang-tools-extra/clangd/Protocol.h
652

could you rather put code after range and message so that we somewhat preserve the ordering. I am not sure if it is used anywhere but looks like we are using an ordered container(std::map) to store these.

nridge marked an inline comment as done.Jun 15 2019, 10:08 PM

The fix actually broke another lit test, because in that test the client was not sending the diagnostic's code back in the codeAction request, resulting in a mismatch with the new comparison function.

I think a real client could potentially could this too (not send the code back), so I revised the fix to address this case, by only using the code in the comparison if it's present in both objects.

Also added the requested new lit test.

nridge updated this revision to Diff 204939.Jun 15 2019, 10:08 PM

Revise fix and add test

kadircet accepted this revision.Jun 17 2019, 1:18 AM

LGTM with one more test case request. Thanks!

clang-tools-extra/clangd/test/fixits-duplication.test
44

could you also add another codeAction request with one of the codes missing?

This revision is now accepted and ready to land.Jun 17 2019, 1:18 AM
nridge updated this revision to Diff 205694.Jun 19 2019, 4:00 PM

Add requested test case

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 4:07 PM