[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Aaron Ballman via Phabricator via cfe-commits]

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