Page MenuHomePhabricator

[ASTImporter] Improve import of FileID.
ClosedPublic

Authored by balazske on Feb 1 2019, 8:09 AM.

Details

Summary

Even if the content cache has a directory and filename, it may be a virtual file.
The old code returned with error in this case, but it is worth to try to handle
the file as it were a memory buffer.

Diff Detail

Repository
rC Clang

Event Timeline

balazske created this revision.Feb 1 2019, 8:09 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

This change is needed for D53818 to make LLDB tests pass.

Ping.

Please raise your objections if you have any until the 4th of March (that date we are going to commit if there are no objections). Also, please let us know if you find this deadline too strict.

I don't know anything about this particular patch, but you aren't allowed to set deadlines like that; the patch review process requires that the patch is actually reviewed. If you aren't getting a response, ask on cfe-dev.

I don't know anything about this particular patch, but you aren't allowed to set deadlines like that; the patch review process requires that the patch is actually reviewed. If you aren't getting a response, ask on cfe-dev.

Okay sorry about that, I will not commit until we get an approve. I think I was mislead by the "LLVM Developer Policy" (https://llvm.org/docs/DeveloperPolicy.html)

Do not assume silent approval, or request active objections to the patch with a deadline.

I interpreted this as after several pings setting a deadline is okay, especially if the patch is small and getting old.
Perhaps the policy should be updated to avoid such misinterpretation in the future.

Unfortunately this part of Clang (ASTImporter) has quite a few active developers other than my colleges. Obviously my colleges could review these patches (and they have done that in our fork) but we thought it would be better to have accept from devs of other organizations. One of the clients of ASTImporter is cross translation unit (CTU) static analysis, the other is LLDB. We hope to have more users and developers of CTU by reaching a point where it is mature enough to attract more developers. At the moment CTU analysis is not successful even on a simple C project like tmux with the upstream master. But that is successful on complex C++ projects like protobuf since a long time in our fork. Upstreaming to master takes painfully too much time. Our fork is already ahead at least 25 commits and it is growing.

shafik accepted this revision.Feb 26 2019, 1:11 PM

LGTM I don't see any LLDB test regressions but I would prefer another reviewer as well.

I am going to be modifying this soon to fix some issues w/ handling built-ins.

This revision is now accepted and ready to land.Feb 26 2019, 1:11 PM

Hi Gabor,
Sorry, missed the patch. It looks mostly good, but do we have any test for this?

balazske added a comment.EditedFeb 27 2019, 3:39 AM

Currently there is no direct test for ASTImporter FileID (and other non-Decl) import. These are tested indirectly by other import tests. There is a failing macOS LLDB test without this fix, after D53818 is applied.

This revision was automatically updated to reflect the committed changes.