This is an archive of the discontinued LLVM Phabricator instance.

FileManager: Improve the FileEntryRef API and customize its OptionalStorage
ClosedPublic

Authored by dexonsmith on Oct 20 2020, 4:19 PM.

Details

Summary

Make a few changes to the FileEntryRef API in preparation for
propagating it enough to remove FileEntry::getName().

  • Allow FileEntryRef to degrade to const FileEntry*. This allows functions currently returning const FileEntry * to be updated to return FileEntryRef without requiring all callers to be updated in the same patch. This helps avoid both (a) massive patches where many fields and locals are updated simultaneously and (b) noisy incremental patches where the first patch adds getFileEntry() at call sites and the second patch removes it.
  • Change operator== to compare the underlying FileEntry*, ignoring any difference to the spelling of the name. There were 0 users of the existing function because it's not useful. This avoids adding noisy code where getFileEntry() is used pervasively to compare equality. In case comparing the named reference becomes important, add (and test) the API now called isSameRef.
  • Allow operator== with const FileEntry * with matching semantics.
  • Customize OptionalStorage<FileEntryRef> to be pointer-sized. Add a private constructor that initializes with nullptr and specialize OptionalStorage to use it.
  • Add OptionalFileEntryRefDegradesToFileEntryPtr, a wrapper around Optional<FileEntryRef> that degrades to const FileEntry*, for the same reason as adding it o FileEntryRef itself. After FileEntryRef has been propagated instances of this should be replaced with Optional<FileEntryRef>.
  • Remove the const from the return of FileEntryRef::getName.
  • Delete the unused FileEntry::isOpenForTests.

Note that there are still FileEntry APIs that aren't wrapped and I
plan to deal with these separately / incrementally, as they are needed.

Diff Detail

Event Timeline

dexonsmith created this revision.Oct 20 2020, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2020, 4:19 PM

Attaching the correct diff :/.

Note that https://reviews.llvm.org/D89836 is an example of a follow-up patch that leverages these API changes. The original version of that patch was much, much noisier.

What's wrong with using Optional<FileEntryRef> instead of MaybeFileEntryRef?

What's wrong with using Optional<FileEntryRef> instead of MaybeFileEntryRef?

Two problems:

  1. const FileEntry* is stored in lots of places. I am nervous after the memory regression from FileEntryRef's introduction (https://reviews.llvm.org/D89580) that doubling the size of these fields will matter. I'd rather save it.
  1. As mentioned in the description, the "degrade to const FileEntry *" behaviour greatly reduces the code churn for incremental patches.

I realize in retrospect I can fix the first problem by customizing OptionalStorage<FileEntryRef>.

IIRC, the follow-up patches got significantly (2x? 5x? it was a lot) smaller when I fixed the second problem. I'm not sure if it's worth it.

dexonsmith retitled this revision from FileManager: Improve the FileEntryRef API and add MaybeFileEntryRef to FileManager: Improve the FileEntryRef API and customize Optional<FileEntryRef>.
dexonsmith edited the summary of this revision. (Show Details)

Dropped MaybeFileEntryRef, instead customizing Optional<FileEntryRef>.

Dropped MaybeFileEntryRef, instead customizing Optional<FileEntryRef>.

As @arphaman pointed out offline, we can actually OptionalStorage instead, by adding add a private constructor to FileEntryRef that nullptr-initializes and only OptionalStorage calls.

The reason I think customizing Optional is better is the ability to add operator const FileEntry *. This avoids having to update large swaths of code like this in an incremental patch:

// old code
lvalue = rvalue;
// new code
auto F = rvalue;
lvalue = F ? &F->getFileEntry() : nullptr;

just to revert it back in a later incremental patch once FileEntryRef has made it further.

This was one the primary reasons I originally added MaybeFileEntryRef.

dexonsmith retitled this revision from FileManager: Improve the FileEntryRef API and customize Optional<FileEntryRef> to FileManager: Improve the FileEntryRef API and customize its OptionalStorage.
dexonsmith edited the summary of this revision. (Show Details)

Okay, here's my third attempt:

  • Add a private constructor to FileEntryRef that sets the pointer to nullptr.
  • Use that in a specialization of OptionalStorage<FileEntryRef>. This makes the size of Optional<FileEntryRef> the same as a pointer.
  • Add OptionalFileEntryRefDegradesToFileEntryPtr, which inherits from Optional<FileEntryRef> and degrades to const FileEntry*. Its name explains exactly what its weird behaviour is, it's easy to grep for, and I'll delete it once FileEntry::getName is gone.

I'll update the two follow-up patches in a moment.

clang-format

arphaman accepted this revision.Oct 28 2020, 8:50 PM

This approach seems like a reasonable compromise, thanks! LGTM.

This revision is now accepted and ready to land.Oct 28 2020, 8:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2020, 10:26 AM
danielkiss added inline comments.
clang/include/clang/Basic/FileEntry.h
152

This broke the build, constexpr could be dropped IMHO.
FileEntry.h(186): error C3615: constexpr function 'llvm::optional_detail::OptionalStorage<clang::FileEntryRef,true>::hasValue' cannot result in a constant expression
FileEntry.h(187): note: failure was caused by call of undefined function or one not declared 'constexpr'
FileEntry.h(187): note: see usage of 'clang::FileEntryRef::hasOptionalValue'

dexonsmith added inline comments.Oct 30 2020, 11:32 AM
clang/include/clang/Basic/FileEntry.h
152

Thanks, I had the same conclusion, I'll drop the constexpr I cargo-culted from the generic version of OptionalStorage.

Note that I already reverted in 940d0a310dca31ae97080b068cef92eadfee6367, if you update you should be able to proceed. (I'm waiting for the bots to fully recover before I push the fixed version.)

Just pushed again in ac49500cd0484e1b2dcf37fa4c0dade6f113c2c9; bots look happier with this version.

Just pushed again in ac49500cd0484e1b2dcf37fa4c0dade6f113c2c9; bots look happier with this version.

Not all of them though; sanitizer-windows is fastest and liked this version better, but I missed until now that ppc64le-lld-multistage-test had a different opinion. I just pushed 814141f9bd0a64bbedae05773972d140f04f654d, which I hope will close this out.