aaron.ballman accepted this revision.
aaron.ballman added a comment.
Found some minor cleanups but otherwise LGTM (feel free to fix when landing if
you'd like).
================
Comment at: clang/tools/libclang/CXType.cpp:380-384
if (const FieldDecl *FD = dyn_cast_or_null<FieldDecl>(D)) {
if (FD->isBitField())
+ return 1;
+ }
+ }
----------------
The conversion is always going to do the right thing:
https://eel.is/c++draft/conv#integral-2.sentence-2
================
Comment at: clang/tools/libclang/CXType.cpp:395
+
+ if (const FieldDecl *FD = dyn_cast_or_null<FieldDecl>(D)) {
+ if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
----------------
================
Comment at: clang/tools/libclang/CXType.cpp:13
+#include "CXType.h"
#include "CIndexer.h"
----------------
dexonsmith wrote:
> vedgy wrote:
> > collinbaker wrote:
> > > vedgy wrote:
> > > > I guess //clang-format// did this include reordering. But it certainly
> > > > looks out of place and the include order becomes wrong. So I think it
> > > > should be reverted.
> > > I don't agree, it's pretty standard for a source file to have its
> > > associated header include at the top.
> > You are right, I haven't realized the header-source association. The diff
> > is still unrelated to the patch. But I'm no longer sure what's right, so
> > won't insist on anything.
> This is correct behaviour from clang-format.
>
> Given that there were no functional changes to includes, typically we’d omit
> clang-format cleanups. (I know there was a change below originally, but that
> was reverted.)
>
> I’m fine leaving the header clean-up in, or separating it out to different
> NFC commit (ideal; could be done when pushing), or skipping it entirely.
+1 -- FWIW, we usually ask for formatting changes to be separated out into a
separate commit because it makes git blame more useful when we're doing code
archeology later, but this is minor enough to be unlikely to cause an issue, so
I don't feel strongly.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130303/new/
https://reviews.llvm.org/D130303
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
