You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi! @andralex asked me to review this paper, so here are some textual suggestions and the following feedback:
The inject_function prefix seems a rather arbitrary choice. I think this needs more explanation for this design choice.
It seems odd to separate the reflection of a function from it's body. Especially since we eventually want to get the body from the reflection of a function. So I would rather expect we have a set_body(info func, info token_seq), and a get_body(info func) -> info
declare_function is not a good name, because calling it does not declare a function in the program yet. This only happens later upon injection.
declare_function: are tokens sufficient? Don't we also need the scope where the function is declared in? Where do expressions naming variables and types for e.g. function default arguments or return type resolve to? I could imagine this being not easily implementable. Am I allowed to use ::foo::bar::f as function name in the token stream? We cannot delay these questions until injection time, since we may want to reflect these properties again before injection happens. Alternatively, we kill this function and carry around plain token stream, as done in P3294.
Can set_qualified_name move a function from one namespace to another? what happens if both namespaces have an equally named type that is used as return value of the function? Does it still refer to the old entities. What if I move the function from one class into another, where certain members are no longer accessible?
I find it odd that friend is classified with the access modifiers. I would separate friendship from access modifiers.
Why not use an enum over the string views for the access modifiers?
There should be a set_attributes. Adding and removing of attributes should be done on the vector returned from attributes(info), so those APIs could be dropped.
Please rename is_pure and set_pure to something like is_pure_virtual etc. to not grab the name pure in case something like __attribute__((pure)) is ever standardized
set_deleted(info) seems to miss a bool parameter and a string for the message.
Missing APIs: get the template parameter list, parameter list, return type, requires clause, noexcept
null_object<T>: I would have expected the proposal to contain an implementation of said class to show that the proposed APIs are sufficient
"Here are a few key components needed:": consider adding: query the body of an existing funciton
The paper references P2996R3, but later in the text refers to P2996R1 a few times. Please consistently use the latest version (R3) for all references.
I have been thinking about get_body(info func) -> info for a while now, and the implementation seems extremely difficult. If the function is only declared, we would have no answer (fine). If the function's body is available, an implementation would either need to retain the token streams of the body for all functions in case they are requested, or reparse the function body upon request. The latter is really expensive because it requires reparsing the entire translation unit to reconstruct the necessary state. But it's solvable. But bugs me recently, are important functions. What if I want to reflect on a function made available by importing a module? No source is available, maybe some form of AST or implementation-specific intermediate representation. We likely don't want to expose that and we probably cannot turn this representation back into a token stream portably.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi! @andralex asked me to review this paper, so here are some textual suggestions and the following feedback:
inject_functionprefix seems a rather arbitrary choice. I think this needs more explanation for this design choice.set_body(info func, info token_seq), and aget_body(info func) -> infodeclare_functionis not a good name, because calling it does not declare a function in the program yet. This only happens later upon injection.declare_function: are tokens sufficient? Don't we also need the scope where the function is declared in? Where do expressions naming variables and types for e.g. function default arguments or return type resolve to? I could imagine this being not easily implementable. Am I allowed to use::foo::bar::fas function name in the token stream? We cannot delay these questions until injection time, since we may want to reflect these properties again before injection happens. Alternatively, we kill this function and carry around plain token stream, as done in P3294.set_qualified_namemove a function from one namespace to another? what happens if both namespaces have an equally named type that is used as return value of the function? Does it still refer to the old entities. What if I move the function from one class into another, where certain members are no longer accessible?friendis classified with the access modifiers. I would separate friendship from access modifiers.set_attributes. Adding and removing of attributes should be done on the vector returned fromattributes(info), so those APIs could be dropped.is_pureandset_pureto something likeis_pure_virtualetc. to not grab the namepurein case something like__attribute__((pure))is ever standardizedset_deleted(info)seems to miss a bool parameter and a string for the message.null_object<T>: I would have expected the proposal to contain an implementation of said class to show that the proposed APIs are sufficient