[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle marked 2 inline comments as done.
malaperle added inline comments.


================
Comment at: clangd/XRefs.cpp:572
+
+  // Try to get the full definition, not just the name
+  SourceLocation StartLoc = Decl.Info->getDefinitionLoc();
----------------
simark wrote:
> hokein wrote:
> > if this is a complicated macro (like `AST_MATCHER`), do we still want to return all the content? they might be less useful than the simple macros.
> I think it would be hard to decide here what constitutes a complicated or not-complicated macro (one for which we want to return all the content or not).  Also, I think it's fine to return the whole content, since frontends generally have good ways to present large contents in popups (hover popups that can be scrolled).
> 
> If we realize later we need to limit the size, then we can add an option where the frontend can specify a size limit.
I think it's OK if the client displays it as it sees fit (scrollbar or trimmed).


================
Comment at: clangd/XRefs.cpp:578
+    bool Invalid;
+    StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
+    if (!Invalid) {
----------------
ilya-biryukov wrote:
> Unfortunately we can't get the buffer here, because for the preamble macros the files might change on disk after we build the preamble and we can end up reading a different version of the file. Which can in turn lead to crashes as the offsets obtained from the source locations can point outside the buffer for the corresponding file.
> 
> This is really annoying and generally the solution is to try find a different way to obtain the same information, e.g. by looking at what `MacroInfo` has available.
> However, I don't know of a good workaround for this. Happy to look into ways of providing something close to a macro definition from `MacroInfo` or other sources, can scout for this tomorrow.
I tried to do something like MacroInfo::dump. One problem is that we don't always have enough information about literals. For example:

```
#define MACRO 0
int main() { return MACRO; }
```
Becomes:
```
#define MACRO numeric_constant
```

I poked around the Token code and I didn't find a way to retrieve the literal without going through the buffer (Preprocessor::getSpelling for example).

What you mention is only a problem when you use the option of storing preambles on disk, right? So something would need to modify the preamble files on disk, which are stored in some temp folder. It doesn't sound like that could happen frequently. There is also bounds checking in the code above so it shouldn't crash, right? Even if the bounds are incorrect, it will be no different than the Range we return to the client for document/definition, i.e. the client can use the range on a newly modified header and get it wrong. I also tested renaming the pch file and then editing the source file and it results in an invalid AST. So either way modifying the pch would be bad news not for just macro hover.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55250/new/

https://reviews.llvm.org/D55250