JornVernee marked 4 inline comments as done. JornVernee added a comment. In D61239#1486005 <https://reviews.llvm.org/D61239#1486005>, @aaron.ballman wrote:
> In D61239#1485994 <https://reviews.llvm.org/D61239#1485994>, @JornVernee > wrote: > > > - holding of on adding helper method until hearing back. > > > My rationale for wanting a helper method is because we already have two > places were incompleteness is important but has special cases. It's easier to > maintain that with something that's a named function to explain what the > predicate is actually doing. My concern is that we add another > `isIncompleteType()` and not think about this issue. > > Do we need this in `validateFieldParentType()`? Thanks for the response, I misunderstood. I usually go with naming my predicates in that way as well, but I misunderstood where you wanted to use it. I think adding the helper method for `validateFieldParentType()` is good. But, the check in `clang_Cursor_getAlignOf` is semantically different, since we basically want to check if the type has an alignment. (I actually realized we also need to check the element type for completeness in the case of incomplete arrays. Figuring that out now). ================ Comment at: test/Index/print-type-size.c:22 + +// RUN: c-index-test -test-print-type-size %s | FileCheck %s +// CHECK: FieldDecl=size:2:9 (Definition) [type=int] [typekind=Int] [sizeof=4] [alignof=4] [offsetof=0] ---------------- aaron.ballman wrote: > This should be the first line of the file. I don't have strong opinions on > where the CHECK lines go, but I usually prefer seeing them near the construct > being checked. This style was taken from `print-type-size.cpp`, which also puts everything at the end. I'll put it at the front/inline instead (makes sense to me as well). I guess it's a legacy thing we're trying to move away from? ================ Comment at: tools/libclang/CXType.cpp:956 QualType FQT = I->getType(); - if (FQT->isIncompleteType()) + if (FQT->isIncompleteType() && !FQT->isIncompleteArrayType()) // IAT is okay here return CXTypeLayoutError_Incomplete; ---------------- aaron.ballman wrote: > aaron.ballman wrote: > > Same here. > I sort of wonder whether we want a helper function like > `isTypeIncompleteForLayout()` or something, and then using that helper > directly. Note that `clang_Type_getSizeOf` will (and AFAIK should) still return `CXTypeLayoutError_Incomplete` for incomplete arrays. I'd expect a potential `isTypeIncompleteForLayout()` to be used by all three functions (OffsetOf, SizeOf, AlignOf). So, I'm in favour of doing without such a helper method for the special OffsetOf and AlignOf cases. It's a pretty unique case (but it appears a bunch of times in Windows API headers so we'd like to have support for it :) ). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61239/new/ https://reviews.llvm.org/D61239 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
