[PATCH] D124221: Add new builtin __builtin_reflect_struct.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I generally just question the usefulness of this.  Despite its shortcomings, the 'dump struct' has the capability of runtime introspection into a type, whereas this seems to require that the user 'know' a significant amount about the type that they are introspecting (number of bases, number of fields, field TYPES). If you knew the type of your struct, THIS well, I would expect you wouldn't NEED a builtin like this.



================
Comment at: clang/docs/LanguageExtensions.rst:2434
+
+     bool print(int arg1, int arg2, std::initializer_list<Field> fields) {
+       for (auto f : fields) {
----------------
I'm curious as to the need for the 'arg1' and 'arg2'?  Also, what is the type of the 'fields' array that works in C?


================
Comment at: clang/docs/LanguageExtensions.rst:2437
+         std::cout << f.name;
+         if (f.bitwidth)
+           std::cout << " : " << f.bitwidth;
----------------
Hmm... what is the type of f.bitwidth?  A zero-length bitwidth is a valid thing, so having it just be an 'int' doesn't seem workable.


================
Comment at: clang/docs/LanguageExtensions.rst:2460
+The ``__builtin_reflect_struct`` function provides simple reflection for a
+class, struct, or union. The first argument of the builtin should be a pointer
+to the type to reflect. The second argument ``f`` should be some callable
----------------
While I get the value of a generic reflection 'thing', this is once again getting awful close to 'just implement reflection capabilities', at which point this seems insufficient.


================
Comment at: clang/docs/LanguageExtensions.rst:2462
+to the type to reflect. The second argument ``f`` should be some callable
+expression, and can be a function object or an overload set. The builtin calls
+``f``, passing any further arguments ``args...`` followed by an initializer
----------------
Is the purpose of the overload set/functor so you can support multiple types (seeing how the 'base' below works)?  How does this all work in C? (or do we just do fields in C, since they cannot have the rest?

I wonder if we would be better if we treated this function as a 'callback' or a 'callback set' that returned a discriminated union of a pre-defined type that describes what is being returned.  That way it could handle member functions, static variables, etc.


================
Comment at: clang/docs/LanguageExtensions.rst:2464
+``f``, passing any further arguments ``args...`` followed by an initializer
+list describing the struct value.
+
----------------
Same question about init-list, how is this usable in C?  It becomes a BIT more usable in C if instead we return a single object that represents a single AST node.


================
Comment at: clang/docs/LanguageExtensions.rst:2471
+
+The initializer list contains the following components:
+
----------------
I find myself wishing this was a more complete list.


================
Comment at: clang/docs/LanguageExtensions.rst:2473
+
+* For each direct base class, the address of the base class, in declaration
+  order.
----------------
I wonder if bases should include their virtualness or access specifier.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:420
+    auto *FD = IFD ? IFD->getAnonField() : dyn_cast<FieldDecl>(D);
+    if (!FD || (FD->isUnnamedBitfield() || FD->isAnonymousStructOrUnion()))
+      continue;
----------------
Losing the unnamed bitfield is unfortunate, and I the 'dump struct' handles.  As is losing the anonymous struct/union.

Also, how does this handle 'record type' members?  Does the user have to know the inner type already?  If they already know that much information about the type, they wouldn't really need this, right?


================
Comment at: clang/test/CodeGenCXX/builtin-reflect-struct.cpp:24
+
+int consume(int, Expected);
+
----------------
OH! I see now.  This is unfortunately quite limiting by doing it this way.  At this point the user of this builtin is required to know the layout of the struct before calling the builtin, which seems unfortunate. Despite the downsides of being just a 'dump function', the other builtin had the benefit of working on an arbitrary type.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221