Page MenuHomePhabricator

Factor out a functionality from `isBeforeInTranslationUnit`
ClosedPublic

Authored by xazax.hun on Jun 22 2017, 4:12 AM.

Details

Summary

Right now source locations from different translation units can not be compared.
This is a problem for an upcoming feature in the Static Analyzer, the cross translation unit support (https://reviews.llvm.org/D30691).

It would be great to be able to sort the source locations. To achieve this I factored out some code from isBeforeInTranslationUnit to be able to determine whether to source locations are from the same translation unit. Note that, in our use-case, these source locations are imported and from the same source manager.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun created this revision.Jun 22 2017, 4:12 AM

Note that, it is not easy to add a test case for this patch without the https://reviews.llvm.org/D30691 being accepted.

joerg edited edge metadata.Jun 22 2017, 7:27 AM

I don't think it is a good idea to make this function non-transitive.

I don't think it is a good idea to make this function non-transitive.

I think this is a good point. However, I am not entirely sure that it is transitive right now. Check the "Both are in built-in buffers, but from different files" case, which is similar to the separate TU case.
But I will look if I can come up with an alternative solution.

akyrtzi edited edge metadata.EditedJun 26 2017, 1:37 PM

Comparing SourceLocations from different translation units is not meaningful and my concern is that treating source locations like this can very easily lead to errors where by mistake the code is resolving a SourceLocation with the wrong translation unit and not the translation unit that it came from.

If you intend to sort source locations across translation units I think you should use an abstraction that includes the source location _and_ the ASTContext or ASTUnit (or something to identify the translation unit) that the source location came from, and use that for sorting (by checking whether the TU are the same, and if not sort appropriately with the TUs, before sorting the SourceLocations).
That way you don't need to 'allow' comparing source locations from different TUs, which IMO is a good sanity check to make sure the code did not mix-up the source locations by accident.

FullSourceLoc could be useful, it wraps the SourceManager that the SourceLocation come from.

Comparing SourceLocations from different translation units is not meaningful and my concern is that treating source locations like this can very easily lead to errors where by mistake the code is resolving a SourceLocation with the wrong translation unit and not the translation unit that it came from.

If you intend to sort source locations across translation units I think you should use an abstraction that includes the source location _and_ the ASTContext or ASTUnit (or something to identify the translation unit) that the source location came from, and use that for sorting (by checking whether the TU are the same, and if not sort appropriately with the TUs, before sorting the SourceLocations).
That way you don't need to 'allow' comparing source locations from different TUs, which IMO is a good sanity check to make sure the code did not mix-up the source locations by accident.

Thank you for the suggestions!
The use-case is to make the analyzer deterministic when reporting the diagnostics. Unfortunately, we are already using FullSourceLocs, and it does not help, because the imported AST (using the ASTImporter) has imported source locations that are using the same source manager as the original AST. But I will look into some workaround.

xazax.hun updated this revision to Diff 104167.Jun 27 2017, 8:00 AM
xazax.hun retitled this revision from Relax an assert in the comparison of source locations to Factor out a functionality from `isBeforeInTranslationUnit`.
xazax.hun edited the summary of this revision. (Show Details)
  • New approach to solve the original problem

I created a new public API that is using a piece of code that was factored out from isBeforeInTranslationUnit. Using this new function it is possible to implement proper comparison of source locations within the Static Analyzer. What do you think?

I'd prefer to avoid including whitespace-only changes (there are a couple of lines in the diff with only whitespace change), otherwise LGTM!

xazax.hun updated this revision to Diff 104358.Jun 28 2017, 1:20 AM
  • Removed the whitespace changes
  • Factored out one more condition

I'd prefer to avoid including whitespace-only changes (there are a couple of lines in the diff with only whitespace change), otherwise LGTM!

Great, thank you! If no one has objections I will commit this tomorrow.

This revision was automatically updated to reflect the committed changes.