Factor out a functionality from `isBeforeInTranslationUnit`

[xazax.hun]

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.

[xazax.hun]

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

[joerg]

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

[xazax.hun]

In D34506#787971, @joerg wrote:

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]

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.

[akyrtzi]

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

[xazax.hun]

In D34506#791089, @akyrtzi wrote:

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]

• New approach to solve the original problem

[xazax.hun]

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?

[akyrtzi]

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]

• Removed the whitespace changes
• Factored out one more condition

[xazax.hun]

In D34506#792313, @akyrtzi wrote:

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.

lib/Basic/SourceManager.cpp

InBeforeInTUCacheEntry &SourceManager::getInBeforeInTUCache(FileID LFID,
   if (I != IBTUCache.end())
     return I->second;
 
   // Fall back to the overflow value.
   return IBTUCacheOverflow;
 }
 
 /// \brief Determines the order of 2 source locations in the translation unit.
+///        It also works when two locations are from different translation
+///        units. In that case it will return *some* order, that is
+///        deterministic for that invocation of the compiler.
 ///
 /// \returns true if LHS source location comes before RHS, false otherwise.
 bool SourceManager::isBeforeInTranslationUnit(SourceLocation LHS,
                                               SourceLocation RHS) const {
   assert(LHS.isValid() && RHS.isValid() && "Passed invalid source location!");
   if (LHS == RHS)
     return false;
 
bool SourceManager::isBeforeInTranslationUnit(SourceLocation LHS,
   bool LIsScratch = LB == "<scratch space>";
   bool RIsScratch = RB == "<scratch space>";
   // Sort scratch after inline asm, but before the rest.
   if (LIsScratch || RIsScratch) {
     if (LIsScratch != RIsScratch)
       return LIsScratch;
     return LOffs.second < ROffs.second;
   }
-  llvm_unreachable("Unsortable locations found");
+  // Source locations from different translation units.
+  return LOffs.first < ROffs.first;
 }
 
 void SourceManager::PrintStats() const {
   llvm::errs() << "\n*** Source Manager Stats:\n";
   llvm::errs() << FileInfos.size() << " files mapped, " << MemBufferInfos.size()
                << " mem buffers mapped.\n";
   llvm::errs() << LocalSLocEntryTable.size() << " local SLocEntry's allocated ("
                << llvm::capacity_in_bytes(LocalSLocEntryTable)