kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:443 + RemoveAll.Annotations.push_back({RemoveAllUnusedID, + {/*label=*/"", /*needsConfirmation=*/true, + /*description=*/std::nullopt}}); ---------------- hokein wrote: > kadircet wrote: > > rather than an empty label, let's duplicate `RemoveAll.Message` here. As > > that context will probably be lost after executing the code action. > I don't see much value on setting the label the the RemoveAll message (the > preview window shows the content diff, it is clear enough for users to > understand the purpose by viewing the diff). > > And since we use one annotation per edit, the `RemoveAll` message doesn't > seem to suit anymore. > I don't see much value on setting the label the the RemoveAll message (the > preview window shows the content diff, it is clear enough for users to > understand the purpose by viewing the diff). i think you're focusing too much on the way VSCode chooses to implement this interaction. Showing the diff in the edit panel is a strategy chosen by VSCode, not per LSP. Spec indicates `label` as `A human-readable string describing the actual change. The string is rendered prominent in the user interface.`. So there's a good chance an editor might chose to just display the `label`. This makes even more sense especially when changes are complicated and don't fit a single line. > And since we use one annotation per edit, the RemoveAll message doesn't seem > to suit anymore. Right, but we actually now have the opportunity to describe each fix properly. We can change the fix message we generate in `generateUnusedIncludeDiagnostics` to contain proper include and use them as annotation labels instead. We already have the logic inside Diagnostics.cpp to synthesize messages for fixes (this would also unify the behaviour), i think we should re-use it here by exposing it in diagnostics.h. WDYT? ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:463 + // FIXME(hokein): emit used symbol reference in the annotation. + ChangeAnnotation Annotation = {/*label=*/"", + /*needsConfirmation=*/true, ---------------- similar to above, let's at least mention the fix properly in label. ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:471 + ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++); + AddAllMissing.Edits.push_back(It.getValue()); + AddAllMissing.Edits.back().annotationId.emplace(ID); ---------------- nit: you can `std::move(It.getValue())` if you drop the `const` from `It`. ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:481 + FixAll.Message = "fix all includes"; + ChangeAnnotation Annotation = {/*label=*/"", + /*needsConfirmation=*/true, ---------------- why are we creating new annotations here? rather than just merging everything from `RemoveAllUnused` and `AddAllMissing` ? ================ Comment at: clang-tools-extra/clangd/Protocol.h:253 + /// The actual annotation identifier. + std::optional<ChangeAnnotationIdentifier> annotationId = std::nullopt; }; ---------------- despite keeping this one as-is, we still don't need to optional, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147684/new/ https://reviews.llvm.org/D147684 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits