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_function
prefix 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) -> 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.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?friend
is 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_pure
andset_pure
to something likeis_pure_virtual
etc. to not grab the namepure
in 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