Skip to content

Improve text of P3157 #171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bernhardmgruber
Copy link

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.

@bernhardmgruber
Copy link
Author

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.

@brevzin brevzin force-pushed the master branch 2 times, most recently from d215d5d to d8dae46 Compare February 13, 2025 11:01
@brevzin brevzin force-pushed the master branch 4 times, most recently from c8f13e1 to a515803 Compare March 31, 2025 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant