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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
