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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
