Implementing P1099R5 (using enum) in clang

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Implementing P1099R5 (using enum) in clang

David Chisnall via cfe-dev
Hi all,

I'm new to Clang, and decided that I want to implement a small C++20 feature - the "using enum" proposal (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html).

The first part of the proposal, supporting syntax such as "using Color::RED" is easy enough to implement, and basically requires removing an if-statement and adding some diagnostics.

However, I struggle finding the correct way to implement the "using enum Color" syntax. There are two approaches that I see when trying to implement it.

The first approach is "expanding" it to a list of using-declarations (i.e "using enum Color" would be equivalent to "using Color::RED, Color::BLUE, ...). This approach is easier to implement, however the resulting AST won't be as accurate.

The second approach is creating a new AST node representing a "UsingEnumDecl". This will generate a more "correct" AST, however it is much more complicated to implement - it is more similar to using directives ("using namespace ns"), whose implementation is complicated and heavily affects the name lookup.

What do you think is the correct way to implement the feature? Is reducing the complexity worth degrading the accuracy of the AST?

Thanks,
Shachaf

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Implementing P1099R5 (using enum) in clang

David Chisnall via cfe-dev
You definitely want to represent the original declaration in the AST — never just expand it right away into the underlying semantics.

Others know the AST much better than I do, but: I think you could *also* implement the easy way as well, by adding implicit UsingShadowDecls for each enumerator, each linked to the original UsingEnumDecl.  Or better yet, somehow add a single shadow decl that is a transparent lookup context, somehow.

I believe UsingShadowDecls are introduced by UsingDecls to handle whenever multiple individual declarations need to be introduced into a context — someone correct me if I’m wrong.  There is definitely precedent for introducing implicit declarations simply for lookup purposes, e.g. VarTemplateDecl instantiations get added to a DeclContext.  It’s annoying to deal with but since it’s already being done, I see no reason not to introducing UsingShadowDecls to handle the implementation in this way.

- Dave

On Jul 10, 2020, at 7:01 AM, Shachaf Co via cfe-dev <[hidden email]> wrote:

Hi all,

I'm new to Clang, and decided that I want to implement a small C++20 feature - the "using enum" proposal (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html).

The first part of the proposal, supporting syntax such as "using Color::RED" is easy enough to implement, and basically requires removing an if-statement and adding some diagnostics.

However, I struggle finding the correct way to implement the "using enum Color" syntax. There are two approaches that I see when trying to implement it.

The first approach is "expanding" it to a list of using-declarations (i.e "using enum Color" would be equivalent to "using Color::RED, Color::BLUE, ...). This approach is easier to implement, however the resulting AST won't be as accurate.

The second approach is creating a new AST node representing a "UsingEnumDecl". This will generate a more "correct" AST, however it is much more complicated to implement - it is more similar to using directives ("using namespace ns"), whose implementation is complicated and heavily affects the name lookup.

What do you think is the correct way to implement the feature? Is reducing the complexity worth degrading the accuracy of the AST?

Thanks,
Shachaf
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Implementing P1099R5 (using enum) in clang

David Chisnall via cfe-dev
On Fri, 10 Jul 2020, 08:39 David Rector via cfe-dev, <[hidden email]> wrote:
You definitely want to represent the original declaration in the AST — never just expand it right away into the underlying semantics.

+1

Others know the AST much better than I do, but: I think you could *also* implement the easy way as well, by adding implicit UsingShadowDecls for each enumerator, each linked to the original UsingEnumDecl.

This is the approach that I would attempt -- model UsingEnumDecl similarly to UsingDecl.

Or better yet, somehow add a single shadow decl that is a transparent lookup context, somehow.

It's tempting to try to build a representation whose size is independent of the number of enumerators, but this is likely to present problems -- name lookup for an injected enumerator name would find a declaration with a different name, which is going to confuse some parts of clang. Also, all name lookups would presumably need to additionally look for the shadow decl, much as we do for "using namespace". We need to pay an O(n) cost to check that the enumerators don't conflict with other declarations in their scope anyway, so building an O(n) representation seems likely to be OK.

I believe UsingShadowDecls are introduced by UsingDecls to handle whenever multiple individual declarations need to be introduced into a context — someone correct me if I’m wrong.  There is definitely precedent for introducing implicit declarations simply for lookup purposes, e.g. VarTemplateDecl instantiations get added to a DeclContext.  It’s annoying to deal with but since it’s already being done, I see no reason not to introducing UsingShadowDecls to handle the implementation in this way.

- Dave

On Jul 10, 2020, at 7:01 AM, Shachaf Co via cfe-dev <[hidden email]> wrote:

Hi all,

I'm new to Clang, and decided that I want to implement a small C++20 feature - the "using enum" proposal (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html).

The first part of the proposal, supporting syntax such as "using Color::RED" is easy enough to implement, and basically requires removing an if-statement and adding some diagnostics.

However, I struggle finding the correct way to implement the "using enum Color" syntax. There are two approaches that I see when trying to implement it.

The first approach is "expanding" it to a list of using-declarations (i.e "using enum Color" would be equivalent to "using Color::RED, Color::BLUE, ...). This approach is easier to implement, however the resulting AST won't be as accurate.

The second approach is creating a new AST node representing a "UsingEnumDecl". This will generate a more "correct" AST, however it is much more complicated to implement - it is more similar to using directives ("using namespace ns"), whose implementation is complicated and heavily affects the name lookup.

What do you think is the correct way to implement the feature? Is reducing the complexity worth degrading the accuracy of the AST?

Thanks,
Shachaf
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Implementing P1099R5 (using enum) in clang

David Chisnall via cfe-dev
Thank you very much!

I'll start looking into the UsingShadowDecls. 
Are there some resources for learning about it, except for clang's documentation and the code itself?
I find the documentation quite lacking, and there are many internal concepts to understand such as Contexts, Scopes, the whole name lookup process etc.

Shachaf

On Fri, Jul 10, 2020 at 7:48 PM Richard Smith <[hidden email]> wrote:
On Fri, 10 Jul 2020, 08:39 David Rector via cfe-dev, <[hidden email]> wrote:
You definitely want to represent the original declaration in the AST — never just expand it right away into the underlying semantics.

+1

Others know the AST much better than I do, but: I think you could *also* implement the easy way as well, by adding implicit UsingShadowDecls for each enumerator, each linked to the original UsingEnumDecl.

This is the approach that I would attempt -- model UsingEnumDecl similarly to UsingDecl.

Or better yet, somehow add a single shadow decl that is a transparent lookup context, somehow.

It's tempting to try to build a representation whose size is independent of the number of enumerators, but this is likely to present problems -- name lookup for an injected enumerator name would find a declaration with a different name, which is going to confuse some parts of clang. Also, all name lookups would presumably need to additionally look for the shadow decl, much as we do for "using namespace". We need to pay an O(n) cost to check that the enumerators don't conflict with other declarations in their scope anyway, so building an O(n) representation seems likely to be OK.

I believe UsingShadowDecls are introduced by UsingDecls to handle whenever multiple individual declarations need to be introduced into a context — someone correct me if I’m wrong.  There is definitely precedent for introducing implicit declarations simply for lookup purposes, e.g. VarTemplateDecl instantiations get added to a DeclContext.  It’s annoying to deal with but since it’s already being done, I see no reason not to introducing UsingShadowDecls to handle the implementation in this way.

- Dave

On Jul 10, 2020, at 7:01 AM, Shachaf Co via cfe-dev <[hidden email]> wrote:

Hi all,

I'm new to Clang, and decided that I want to implement a small C++20 feature - the "using enum" proposal (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html).

The first part of the proposal, supporting syntax such as "using Color::RED" is easy enough to implement, and basically requires removing an if-statement and adding some diagnostics.

However, I struggle finding the correct way to implement the "using enum Color" syntax. There are two approaches that I see when trying to implement it.

The first approach is "expanding" it to a list of using-declarations (i.e "using enum Color" would be equivalent to "using Color::RED, Color::BLUE, ...). This approach is easier to implement, however the resulting AST won't be as accurate.

The second approach is creating a new AST node representing a "UsingEnumDecl". This will generate a more "correct" AST, however it is much more complicated to implement - it is more similar to using directives ("using namespace ns"), whose implementation is complicated and heavily affects the name lookup.

What do you think is the correct way to implement the feature? Is reducing the complexity worth degrading the accuracy of the AST?

Thanks,
Shachaf
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Implementing P1099R5 (using enum) in clang

David Chisnall via cfe-dev
I’m afraid the best thing to do is look at the code - the AST documentation is often not quite clear, and sometimes is entirely absent for important stuff, but on the other hand the code is usually very clear, and you really do learn a lot by looking at precisely how something was implemented.

You should not need to worry about scopes and lookup, just create UsingShadowDecls for each of the EnumeratorDecls of the enum; other than that, as Richard suggested, just follow exactly how UsingDecl does it.  That is the best way to learn, too.

Note you’ll also need an UnresolvedUsingEnumDecl for e.g. "using enum T::someenum" when T is dependent; in TreeTransfomr::… you may transform that to your UsingEnumDecl when T becomes non-dependent; follow the example of e.g. UnresolvedUsingTypenameDecl I believe it is.

Good luck,

Dave

On Jul 10, 2020, at 1:33 PM, Shachaf Co <[hidden email]> wrote:

Thank you very much!

I'll start looking into the UsingShadowDecls. 
Are there some resources for learning about it, except for clang's documentation and the code itself?
I find the documentation quite lacking, and there are many internal concepts to understand such as Contexts, Scopes, the whole name lookup process etc.

Shachaf

On Fri, Jul 10, 2020 at 7:48 PM Richard Smith <[hidden email]> wrote:
On Fri, 10 Jul 2020, 08:39 David Rector via cfe-dev, <[hidden email]> wrote:
You definitely want to represent the original declaration in the AST — never just expand it right away into the underlying semantics.

+1

Others know the AST much better than I do, but: I think you could *also* implement the easy way as well, by adding implicit UsingShadowDecls for each enumerator, each linked to the original UsingEnumDecl.

This is the approach that I would attempt -- model UsingEnumDecl similarly to UsingDecl.

Or better yet, somehow add a single shadow decl that is a transparent lookup context, somehow.

It's tempting to try to build a representation whose size is independent of the number of enumerators, but this is likely to present problems -- name lookup for an injected enumerator name would find a declaration with a different name, which is going to confuse some parts of clang. Also, all name lookups would presumably need to additionally look for the shadow decl, much as we do for "using namespace". We need to pay an O(n) cost to check that the enumerators don't conflict with other declarations in their scope anyway, so building an O(n) representation seems likely to be OK.

I believe UsingShadowDecls are introduced by UsingDecls to handle whenever multiple individual declarations need to be introduced into a context — someone correct me if I’m wrong.  There is definitely precedent for introducing implicit declarations simply for lookup purposes, e.g. VarTemplateDecl instantiations get added to a DeclContext.  It’s annoying to deal with but since it’s already being done, I see no reason not to introducing UsingShadowDecls to handle the implementation in this way.

- Dave

On Jul 10, 2020, at 7:01 AM, Shachaf Co via cfe-dev <[hidden email]> wrote:

Hi all,

I'm new to Clang, and decided that I want to implement a small C++20 feature - the "using enum" proposal (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html).

The first part of the proposal, supporting syntax such as "using Color::RED" is easy enough to implement, and basically requires removing an if-statement and adding some diagnostics.

However, I struggle finding the correct way to implement the "using enum Color" syntax. There are two approaches that I see when trying to implement it.

The first approach is "expanding" it to a list of using-declarations (i.e "using enum Color" would be equivalent to "using Color::RED, Color::BLUE, ...). This approach is easier to implement, however the resulting AST won't be as accurate.

The second approach is creating a new AST node representing a "UsingEnumDecl". This will generate a more "correct" AST, however it is much more complicated to implement - it is more similar to using directives ("using namespace ns"), whose implementation is complicated and heavily affects the name lookup.

What do you think is the correct way to implement the feature? Is reducing the complexity worth degrading the accuracy of the AST?

Thanks,
Shachaf
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Implementing P1099R5 (using enum) in clang

David Chisnall via cfe-dev
On Fri, 10 Jul 2020 at 15:09, David Rector via cfe-dev <[hidden email]> wrote:
I’m afraid the best thing to do is look at the code - the AST documentation is often not quite clear, and sometimes is entirely absent for important stuff, but on the other hand the code is usually very clear, and you really do learn a lot by looking at precisely how something was implemented.

You should not need to worry about scopes and lookup, just create UsingShadowDecls for each of the EnumeratorDecls of the enum; other than that, as Richard suggested, just follow exactly how UsingDecl does it.  That is the best way to learn, too.

Note you’ll also need an UnresolvedUsingEnumDecl for e.g. "using enum T::someenum" when T is dependent; in TreeTransfomr::… you may transform that to your UsingEnumDecl when T becomes non-dependent; follow the example of e.g. UnresolvedUsingTypenameDecl I believe it is.

We don't need that in this case, because the language rules don't permit `using enum A::B;` to name a dependent type. (The reason it's disallowed is that it would create major implementation problems -- we couldn't reasonably do unqualified lookups in a scope that contains such a declaration because we'd have no idea if the name resolves to an enumerator.)
 
Good luck,

Dave

On Jul 10, 2020, at 1:33 PM, Shachaf Co <[hidden email]> wrote:

Thank you very much!

I'll start looking into the UsingShadowDecls. 
Are there some resources for learning about it, except for clang's documentation and the code itself?
I find the documentation quite lacking, and there are many internal concepts to understand such as Contexts, Scopes, the whole name lookup process etc.

Shachaf

On Fri, Jul 10, 2020 at 7:48 PM Richard Smith <[hidden email]> wrote:
On Fri, 10 Jul 2020, 08:39 David Rector via cfe-dev, <[hidden email]> wrote:
You definitely want to represent the original declaration in the AST — never just expand it right away into the underlying semantics.

+1

Others know the AST much better than I do, but: I think you could *also* implement the easy way as well, by adding implicit UsingShadowDecls for each enumerator, each linked to the original UsingEnumDecl.

This is the approach that I would attempt -- model UsingEnumDecl similarly to UsingDecl.

Or better yet, somehow add a single shadow decl that is a transparent lookup context, somehow.

It's tempting to try to build a representation whose size is independent of the number of enumerators, but this is likely to present problems -- name lookup for an injected enumerator name would find a declaration with a different name, which is going to confuse some parts of clang. Also, all name lookups would presumably need to additionally look for the shadow decl, much as we do for "using namespace". We need to pay an O(n) cost to check that the enumerators don't conflict with other declarations in their scope anyway, so building an O(n) representation seems likely to be OK.

I believe UsingShadowDecls are introduced by UsingDecls to handle whenever multiple individual declarations need to be introduced into a context — someone correct me if I’m wrong.  There is definitely precedent for introducing implicit declarations simply for lookup purposes, e.g. VarTemplateDecl instantiations get added to a DeclContext.  It’s annoying to deal with but since it’s already being done, I see no reason not to introducing UsingShadowDecls to handle the implementation in this way.

- Dave

On Jul 10, 2020, at 7:01 AM, Shachaf Co via cfe-dev <[hidden email]> wrote:

Hi all,

I'm new to Clang, and decided that I want to implement a small C++20 feature - the "using enum" proposal (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html).

The first part of the proposal, supporting syntax such as "using Color::RED" is easy enough to implement, and basically requires removing an if-statement and adding some diagnostics.

However, I struggle finding the correct way to implement the "using enum Color" syntax. There are two approaches that I see when trying to implement it.

The first approach is "expanding" it to a list of using-declarations (i.e "using enum Color" would be equivalent to "using Color::RED, Color::BLUE, ...). This approach is easier to implement, however the resulting AST won't be as accurate.

The second approach is creating a new AST node representing a "UsingEnumDecl". This will generate a more "correct" AST, however it is much more complicated to implement - it is more similar to using directives ("using namespace ns"), whose implementation is complicated and heavily affects the name lookup.

What do you think is the correct way to implement the feature? Is reducing the complexity worth degrading the accuracy of the AST?

Thanks,
Shachaf
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Implementing P1099R5 (using enum) in clang

David Chisnall via cfe-dev


On Jul 10, 2020, at 6:36 PM, Richard Smith <[hidden email]> wrote:

On Fri, 10 Jul 2020 at 15:09, David Rector via cfe-dev <[hidden email]> wrote:
...
Note you’ll also need an UnresolvedUsingEnumDecl for e.g. "using enum T::someenum" when T is dependent; in TreeTransfomr::… you may transform that to your UsingEnumDecl when T becomes non-dependent; follow the example of e.g. UnresolvedUsingTypenameDecl I believe it is.

We don't need that in this case, because the language rules don't permit `using enum A::B;` to name a dependent type. (The reason it's disallowed is that it would create major implementation problems -- we couldn't reasonably do unqualified lookups in a scope that contains such a declaration because we'd have no idea if the name resolves to an enumerator.)

Is that rule really necessary though?  Why not allow the dependent enum itself and let lookup fail for the enumerators, just until it finally is transformed into a resolved UsingEnumDecl?  E.g.:

```
enum class fruit  { orange, apple };
enum class fruitB { orange, apple, pear };

template<typename FRUIT_IMPL>
struct S {
  using enum FRUIT_IMPL; //why does this need to be an error?
  static const auto BadVal = orange; //Definitely an error, undeclared ID "orange"; shouldn’t this suffice?
  static const auto GoodVal = FRUIT_IMPL::orange; // This should be okay though, since still dependent.
};

void f() {
  S<fruit> s;
  s.orange;

  S<fruitB> s2;
  s2.pear;
}
```

I couldn’t find an explanation on the page Shachaf referenced.  In any case, if there is no need to handle a dependent case this should be very straightforward, Shachaf.  Good luck,

- Dave


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Implementing P1099R5 (using enum) in clang

David Chisnall via cfe-dev
Scratch that, I already see the problem: suppose one of those enums was not scoped, i.e. 

enum fruit { orange, apple };
enum class fruitB { apple, orange, pear; };

template<typename FRUIT_IMPL>
struct S {
  using enum FRUIT_IMPL; // really is bad, because...
  static const auto val = orange; // this would be resolved to fruit::orange, not FRUIT_IMPL::orange
};

void f() {
  S<fruitB> s2;
  s2.orange //refers to fruit::orange, not fruitB::orange, whoops
}

So no dependent case of UsingEnumDecl after all.  Thanks Richard and good luck Shachaf,

- Dave 


On Jul 10, 2020, at 7:55 PM, David Rector <[hidden email]> wrote:



On Jul 10, 2020, at 6:36 PM, Richard Smith <[hidden email]> wrote:

On Fri, 10 Jul 2020 at 15:09, David Rector via cfe-dev <[hidden email]> wrote:
...
Note you’ll also need an UnresolvedUsingEnumDecl for e.g. "using enum T::someenum" when T is dependent; in TreeTransfomr::… you may transform that to your UsingEnumDecl when T becomes non-dependent; follow the example of e.g. UnresolvedUsingTypenameDecl I believe it is.

We don't need that in this case, because the language rules don't permit `using enum A::B;` to name a dependent type. (The reason it's disallowed is that it would create major implementation problems -- we couldn't reasonably do unqualified lookups in a scope that contains such a declaration because we'd have no idea if the name resolves to an enumerator.)

Is that rule really necessary though?  Why not allow the dependent enum itself and let lookup fail for the enumerators, just until it finally is transformed into a resolved UsingEnumDecl?  E.g.:

```
enum class fruit  { orange, apple };
enum class fruitB { orange, apple, pear };

template<typename FRUIT_IMPL>
struct S {
  using enum FRUIT_IMPL; //why does this need to be an error?
  static const auto BadVal = orange; //Definitely an error, undeclared ID "orange"; shouldn’t this suffice?
  static const auto GoodVal = FRUIT_IMPL::orange; // This should be okay though, since still dependent.
};

void f() {
  S<fruit> s;
  s.orange;

  S<fruitB> s2;
  s2.pear;
}
```

I couldn’t find an explanation on the page Shachaf referenced.  In any case, if there is no need to handle a dependent case this should be very straightforward, Shachaf.  Good luck,

- Dave



_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev