void added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:4246
+ private:
+ SourceRange countedByFieldLoc;
+ public:
----------------
aaron.ballman wrote:
> void wrote:
> > erichkeane wrote:
> > > aaron.ballman wrote:
> > > > Teeny tiniest of coding style nits :-D
> > > Should we instead be capturing the field itself, rather than its
> > > location? It seems to me that would be more useful?
> > I tried that before and it didn't work. The issue is that at the time of
> > parsing we don't yet have the full definition of the structure / union. So
> > the `Decl`'s aren't really available to us yet. There may be a way to
> > massage the parsing code to allow this to happen, but I'd like to do it as
> > a separate patch. I'll document it with a `FIXME`.
> I think at some point we're going to want this to take an `Expr` argument
> instead of an identifier argument, so we can support `foo.bar` and `baz[0]`,
> etc as arguments for more complicated situations. But I believe the current
> form is reasonable as-is (we can start taking an expression and all of these
> identifiers should become a `MemberExpr`, so users don't have to change their
> code).
I think that GCC may not be able to use expressions (apart from simple,
resolvable integer arithmetic). But yeah, those types of expansions would be
nice. There are many issues to resolve with them first---such as how to
reference a field in a sub-struct when the fam is in another substruct:
```
struct S {
struct A {
int count;
} a;
struct C {
int count;
struct X {
int count;
} x;
} c;
struct F {
int fam[] __counted_by(/* how to refer to p->c.x.count? */);
} f;
};
```
================
Comment at: clang/lib/Sema/SemaDecl.cpp:18029
+
+ S.LookupName(Result, Scope);
+ if (Result.getResultKind() == LookupResult::Found) {
----------------
aaron.ballman wrote:
> When the lookup result is not found, I think you should call
> `Sema::DiagnoseEmptyLookup()`. This will handle giving you the error about
> the member not being found, but it should also give you typo correction for
> free, and if typo correction happens, you should then already know what new
> declaration was found via the correction, so you can continue to check for
> the other cases from there.
>
> https://github.com/llvm/llvm-project/blob/b57df9fe9a1a230f277d671bfa0884bbda9fc1c5/clang/lib/Sema/SemaLambda.cpp#L1151
> is an example of this being called elsewhere.
GAAAA! It's just like the rest of Clang: There are a million ways to do
something, but none of them are documented and only one of them works for your
particular case...kind of...and it takes a ton of arguments which seem
counter-intuitive (`CXXScopeSpec` instead of `DeclSpec`?). All of Clang has
been over-engineered from the beginning and it shows in how hard it is to do
even the most basic things. So hard that people write the same things over and
over with only small changes (see the two designated initializers
implementations) leading to a nightmare.
The example you pointed to doesn't work. And none of the other forms seem to
fit well enough. There are apparently many ways to "correct" a typo, but none
of them seemed appropriate or like they'd do what I want. Is it really
necessary to have this?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:18035
+ << ECA->getCountedByField() << SR;
+ } else if (!Field->getType()->isIntegerType()) {
+ // The "counted_by" field must have an integer type.
----------------
aaron.ballman wrote:
> void wrote:
> > aaron.ballman wrote:
> > > Errr any integer type?
> > > ```
> > > struct S {
> > > bool HerpADerp;
> > > int FAM[] __attribute__((counted_by(HerpADerp)));
> > > };
> > > ```
> > > seems like something we don't want, but I also wonder about enumeration
> > > types and more interestingly, plain `char` where it could be treated as
> > > signed or unsigned depending on compiler flags.
> > Yeah...unless they want just 1 element. ;-) I can rule a `bool` out. (Done)
> >
> > A `char` should be fine. Dunno about signed vs. unsigned...
> The `char` situation I'm thinking about is morally:
> https://godbolt.org/z/377WPYojz
>
> With `-fsigned-char`, accessing `Idx` means you get the value `-1` and with
> `-funsigned-char`, you get the value `255`. If that was used to index into
> `FAM`, the `signed char` version would become a bounds access issue whereas
> the `unsigned char` version works fine, so it's hard to write a portable
> attribute that refers to a `char` member.
>
> Then again, the `counted_by` attribute is intended to catch exactly this kind
> of issue, so perhaps it's fine on `char`?
True. But we technically have that issue with every type. Maybe it'd be best to
make sure we don't have a negative result?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148381/new/
https://reviews.llvm.org/D148381
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
