[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. In D127082#4101322 , @daiyousei-qz wrote: > In D127082#4089377 , @nridge wrote: > >> @daiyousei-qz what is the current status of this patch? Is it ready to be >> merged again? (If so, I

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz added a comment. In D127082#4089377 , @nridge wrote: > @daiyousei-qz what is the current status of this patch? Is it ready to be > merged again? (If so, I can do that for you.) Honestly, I don't know. I looked at TOT llvm repo in github

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. @daiyousei-qz what is the current status of this patch? Is it ready to be merged again? (If so, I can do that for you.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

vitalybuka added a comment. 897.65s: Clangd Unit Tests :: ./ClangdTests/25/72 In D127082#3776206 , @daiyousei-qz wrote: > In D127082#3776173 , @vitalybuka > wrote: > >> Looks like it breaks >>

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz added a comment. In D127082#3776173 , @vitalybuka wrote: > Looks like it breaks > https://lab.llvm.org/buildbot/#/builders/236/builds/206 > https://lab.llvm.org/buildbot/#/builders/237/builds/85 >

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

vitalybuka added a comment. Looks like it breaks https://lab.llvm.org/buildbot/#/builders/236/builds/206 https://lab.llvm.org/buildbot/#/builders/237/builds/85 https://lab.llvm.org/buildbot/#/builders/238/builds/103 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG44bbf20965d2: [clangd] Add Macro Expansion to Hover (authored by daiyousei-qz, committed by sammccall). Changed prior to

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz added a comment. In D127082#3770763 , @sammccall wrote: > Sorry for the delay :-( This looks great! > > It looks like this is your first patch, so you don't have commit access. I > can land the patch for you. > Can you provide an email

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

sammccall added a comment. Sorry for the delay :-( This looks great! It looks like this is your first patch, so you don't have commit access. I can land the patch for you. Can you provide an email address for attribution? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz added a comment. @sammccall Hello, is there any update required by me? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 ___ cfe-commits mailing

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 447962. daiyousei-qz added a comment. remove dedicated MacroExpansion variable (reuse Definition) added empty MACRO test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:1187 - if (!Definition.empty()) { + if (!Definition.empty() || !MacroExpansion.empty()) { Output.addRuler(); sammccall wrote: > Sorry, I think I wasn't clear about what I

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

sammccall added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:1187 - if (!Definition.empty()) { + if (!Definition.empty() || !MacroExpansion.empty()) { Output.addRuler(); Sorry, I think I wasn't clear about what I was asking for...

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz marked 5 inline comments as done. daiyousei-qz added a comment. I have also moved the expansion out of the location checking branch. I think it's to find out if a macro actually has a definition which isn't always the case especially for special macros like `__FILE__`. Here's the

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 446041. daiyousei-qz added a comment. resolve review comment produce expansion even if definition isn't available Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

sammccall added a comment. Sorry, didn't realize this was waiting on me! I like the feature and the threshold seems like a good idea. People might find it inconsistent, but we'll have to see. I like placing it after the macro definition, I think the cases where expansion is/isn't shown feel

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 444151. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 Files: clang-tools-extra/clangd/Hover.cpp clang-tools-extra/clangd/Hover.h

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 444150. daiyousei-qz added a comment. Herald added subscribers: Sanitizers, Enna1. Herald added a project: Sanitizers. fix patch context Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 444147. daiyousei-qz added a comment. fix line end to LF Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 Files: clang-tools-extra/clangd/Hover.cpp

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:1091 + + // Reformat Macro Expansion + if (!HI->MacroExpansion.empty()) { nridge wrote: > daiyousei-qz wrote: > > nridge wrote: > > > It would be interesting to have a couple of test

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. I fixed it locally by running `dos2unix` on the patch before applying. However, if you're able to, please update the patch to remove the Windows line endings so it can be applied with `arc patch`, otherwise the patch metadata is lost during manual application.

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. I would like to apply the updated patch to try it some more, but `arc patch` is failing for me. Applying the raw diff manually with `patch` also fails with the following errors: $ patch -p1 < ../patches/macro-hover.patch patching file

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz added a comment. It looks like my inline comment wasn't submitted (didn't click the submit button in the bottom). Here's my old comment. Comment at: clang-tools-extra/clangd/Hover.cpp:1091 + + // Reformat Macro Expansion + if (!HI->MacroExpansion.empty()) {

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 442416. daiyousei-qz marked an inline comment as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 Files: clang-tools-extra/clangd/Hover.cpp

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:1091 + + // Reformat Macro Expansion + if (!HI->MacroExpansion.empty()) { nridge wrote: > It would be interesting to have a couple of test cases that exercise the > reformatting in

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 441920. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 Files: clang-tools-extra/clangd/Hover.cpp clang-tools-extra/clangd/Hover.h

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. Thanks for the update! In D127082#3606746 , @daiyousei-qz wrote: > Therefore, I still put definition in front of the expansion in this update. > > Regarding to the size limit of expansion: > Here I'm using a 2048 bytes buffer.

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 439592. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 Files: clang-tools-extra/clangd/Hover.cpp clang-tools-extra/clangd/Hover.h

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 439591. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 Files: clang-tools-extra/clangd/Hover.cpp clang-tools-extra/clangd/Hover.h

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 439590. daiyousei-qz added a comment. use a rebased patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 Files: clang-tools-extra/clangd/Hover.cpp

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz added a comment. Sorry I'm a little occupied lately. Regarding to the order of `Definition` and `MacroExpansion`, I experimented putting expansion first and noticed the following: 1. Short definition and short expansion: order doesn't matter, it's always clear 2. Short definition

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz updated this revision to Diff 439585. daiyousei-qz added a comment. - add context to patch - use a fixed size buffer for expansion text Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127082/new/ https://reviews.llvm.org/D127082 Files:

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. In D127082#3560642 , @daiyousei-qz wrote: > Basically, anywhere the same code action isn't available. Need this be > documented somewhere? If you'd like, we could mention the ability to show macro expansions here

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

nridge added a comment. Thank you for working on this! I've been using clangd with this patch applied for a week or so, and it seems to be working well, I haven't run into any issues. Could you please upload a patch with context, to make it easier to review? (Currently, above each hunk it

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

daiyousei-qz added a comment. To elaborate on known situation where expansion isn't available: 1. expansion doesn't work on site of #define 2. expansion doesn't work for _Pragma(...) and alike 3. expansion doesn't work with builtin macro 4. expansion doesn't work for macros as arguments of