ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. See the nit about naming of an error, though
================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:429
"error opening file '%0': %1">, DefaultFatal;
+def err_pp_including_mainfile_for_preamble : Error<
+ "main file cannot be included recursively when building a preamble">;
----------------
NIT: Maybe change to err_pp_including_mainfile_**in**_preamble?
================
Comment at: lib/Basic/SourceManager.cpp:1594
SourceFileName = llvm::sys::path::filename(SourceFile->getName());
- if (*SourceFileName == llvm::sys::path::filename(MainFile->getName()))
{
+ if (MainFile && *SourceFileName ==
llvm::sys::path::filename(MainFile->getName())) {
SourceFileUID = getActualFileUID(SourceFile);
----------------
nik wrote:
> ilya-biryukov wrote:
> > nik wrote:
> > > ilya-biryukov wrote:
> > > > Can this actually be`null`?
> > > The comment for OrigEntry states that it might be null:
> > >
> > > /// Reference to the file entry representing this ContentCache.
> > > ///
> > > /// This reference does not own the FileEntry object.
> > > ///
> > > /// It is possible for this to be NULL if the ContentCache
> > > encapsulates
> > > /// an imaginary text buffer.
> > > const FileEntry *OrigEntry;
> > >
> > > Note also that further down in this function, a null check for
> > > MainContentCache->OrigEntry is done, so I believe that this was just
> > > forgotten here (since there is also no assert) and we are the first one
> > > running into this with the introduced call to
> > > SourceMgr.translateFile(File).
> > >
> > > I've tried to understand why we end up with a nullptr here, but this goes
> > > beyond me in the time I've taken for this. What I've observed is that
> > > module code path (LibclangReparseTest.ReparseWithModule vs
> > > LibclangReparseTest.Reparse) creates at least a MemoryBuffer more (in
> > > ModuleMap::parseModuleMapFile; possibly the one referred with "imaginary
> > > text buffer" from the comment) and I suspect this to be somehow related
> > > with the OrigEntry nullptr.
> > Should we check for equality of `MainContentCache->ContentsEntry` in that
> > case? I guess this is what should be set for "imaginary" files.
> Does not help in this particular case as ContentsEntry is also a nullptr. All
> the members of the ContentsCache seem to be either nullptr or 0 as that
> particular point of execution.
>
> How to continue?
>
> a) Accept as is.
> b) Ask someone knowledgeable for whom this could be a no-brainer that could
> respond hopefully soon. Any candidate?
> c) Or should I dig deeper? This can take quite some time as this area is new
> for me.
>
Wow, I guess I don't understand this code deep enough.
Your change looks ok, I'm happy to LGTM this. But if you're actually interested
in digging deeper, that would be super-useful.
Based on what you're describe, it feels like "imaginary" main file works by
accident rather than being properly handled in this function.
BTW, thanks for moving the check up, it definitely looks nicer this way.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53866/new/
https://reviews.llvm.org/D53866
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
