[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Basic/FileEntry.h:117
     /// gcc5.3.  Once that's no longer supported, change this back.
-    llvm::PointerUnion<FileEntry *, const void *> V;
+    llvm::PointerUnion<const FileEntry *, const void *> V;
 
----------------
dexonsmith wrote:
> These `const`-ness changes look independent and could land separately / ahead of time as NFC changes, independently of `isValid()` and whatever we end up with for FileEntryTest.
Sure. In it's current form the patch requires these changes as we can't get a mutable FileEntry*.

I'll split them out.


================
Comment at: clang/lib/Basic/FileManager.cpp:462
   UFE->Dir     = &DirInfo->getDirEntry();
-  UFE->UID     = NextFileUID++;
-  UFE->IsValid = true;
+  UFE->UID = NextFileUID++;
   UFE->File.reset();
----------------
dexonsmith wrote:
> sammccall wrote:
> > kadircet wrote:
> > > sammccall wrote:
> > > > kadircet wrote:
> > > > > nit: revert formatting
> > > > `arc` insists on this change here, because it's in the blast radius of the next line and clang-format isn't configured to allow this formatting.
> > > > 
> > > > Unless you feel strongly I'd rather not fight the linter when it's not really wrong.
> > > > (Happy to reformat the other lines to match though)
> > > > Unless you feel strongly I'd rather not fight the linter when it's not really wrong.
> > > > (Happy to reformat the other lines to match though)
> > > 
> > > I was suggesting this for preserving history purposes, in theory all of this is old code with intricate semantics so it would be nice to not add more commits to the history chain here. I think it should be possible to just `arc diff --nolint` (or push without updating the diff in phabricator, which you already probably know, but just in case).
> > Sure, fair enough
> > `arc` insists on this change here, because it's in the blast radius of the next line and clang-format isn't configured to allow this formatting.
> > 
> > Unless you feel strongly I'd rather not fight the linter when it's not really wrong.
> > (Happy to reformat the other lines to match though)
> 
> I know this was already reverted / dealt with (SGTM; I agree with @kadircet, although probably I feel less strongly), but I'm wondering if there's a bug in `arc` or `clang-format` to report or fix. Seems to me like this *shouldn't* be considered part of the blast radius. Why should deleting a line trigger reformatting the lines before/after, when those lines are not part of the same construct/statement?
I happen to know this one :-)
Clang-format can support alignment between lines (e.g. of `=`). Removing line 2 from well-formatted code *can* cause spaces to need to be removed from line 1.
In this case, `AlignConsecutiveAssignments` is false in LLVM style, and the spaces were removed for an unrelated reason. But clang-format doesn't track causality. Getting it to preserve misformatting adjacent to modified code would be a bit of a project!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123197/new/

https://reviews.llvm.org/D123197