adambadura / type_name

C++17-compatible, compile-time type name retrieval library.
Apache License 2.0
13 stars 5 forks source link

GCC code, etc. doesn't handle array types correctly #3

Open HexadigmSystems opened 1 year ago

HexadigmSystems commented 1 year ago

Took a quick look at your GCC code out of curiosity (I've already got my own equivalent code for various compilers) and it won't handle square brackets in array types for obvious reasons. Easy fix though (didn't look at any of your other implementations).

BTW, for GCC (not Clang), if someone changes the "auto" return type of "type_name::get()" to, say, "std::string_view" (which some might do if they don't like "auto"), it will also break (for GCC specifically). That's because the type portion of "[with T = " will then end with a semicolon instead of a square bracket. You'll see why if you check it. Just letting you know though you may (understandably) not care (since people who change it do so at their own risk but I'd argue it's better to make the code resistant to any changes in the function's signature).

adambadura commented 1 year ago

Thanks!

There is already another pending issue I should fix - if ']' is used as the template argument (including also numerical values like 93 or 0x5D from ASCII) it will break the "algorithm" of detecting the end of the type part. See https://godbolt.org/z/nKPscd3Pe.

HexadigmSystems commented 1 year ago

Yes, and one other thing to consider as well, though nothing to do with this issue. Will post it as separate issue.

adambadura commented 1 year ago

The problem with ] being part of the type name (whether by the array as in your sample or by the value of a template argument as in mine) is actually quite easy to fix. All it gets is to replace

constexpr auto end_index = full_name.find(right_marker, left_marker_index);

with

constexpr auto end_index = full_name.rfind(right_marker);

In fact, perhaps we should also replace the subsequent:

static_assert(end_index != std::string_view::npos);

with a more restrictive:

static_assert((end_index != std::string_view::npos) && (end_index + right_marker.size() == full_name.size()));

since it better expresses the assumption about the contents of the __PRETTY_FUNCTION__ and should lead to early issue detection instead of producing wrong strings.

adambadura commented 1 year ago

The change of auto to std::string_view is a worse one. The key part of the __PRETTY_FUNCTION__ changes from:

... [with T = <name>]

into

... [with T = <name>; std::string_view = std::basic_string_view<char>]

To handle that I would need to add extra code to conditionally search for ; and then also further complication the handle the case ';' turns out to be a template argument.

I'm not sure if I will be willing to address this problem. Especially, that GCC user unwilling to use auto can also replace with std::basic_string_view<char> (instead of std::string_view) and the problem goes away. I will deliberate more on this problem.

adambadura commented 1 year ago

Actually, a GCC user changing get from returning auto to returning std::string_view (I expect similarly so with value_type after order changes) can also change the right_marker from just ] to the ; std::string_view...] and all should work as before. If a user decided to change one thing in the internals of the implementation they can very well change the other one as well.

I don't think I will cover it in the code, but I might drop a comment or something.

HexadigmSystems commented 1 year ago

Yes, already aware of the issues you mentioned. Fixes are easy as you pointed out. It's a judgment call about how to handle things and entirely yours of course. FWIW, in my own code (written a long time ago and a bit different from yours - just one implementation and one header with #defined constants that identify the target compiler - multiple headers have some drawbacks IMHO I won't get into here), I check for the semicolon and if not found, I look for the ending square bracket using "rfind()". IMHO it's more robust for code that's publicly posted (and even not). Developers will often download code, change names, etc. It's just easier if the code won't break by doing so, and you're not forced to study the implementation to fix it (just change the name and you're done). In this case the code is minimal and simple enough to fix however so not a big deal (and making it more robust against name changes does have the disadvantage of being more complicated, though again, it's minimal in this case).

BTW, I also have a "static_assert" in my own code just after the implementation. It arbitrarily checks that passing float returns "float", though any type whose name is consistent among all target compilers will obviously do. It's quick and dirty but given the undocumented and therefore (potentially) brittle nature of PRETTYFUNCTION and FUNCSIG, it provides one final layer of safety (in addition to the other "static_asserts" I have)

It's a wonder that in 2023, there's nothing in the standard that does this yet (returns a type's WYSIWYG name). Names returned by "type_info::name" aren't always clean, and it's also runtime only (in light of the movement of so much in C++ towards "constexpr" techniques in recent years).

adambadura commented 1 year ago

just one implementation and one header with #defined constants that identify the target compiler - multiple headers have some drawbacks IMHO I won't get into here

I wanted to make a "proper library". But in retrospect, I'm more skeptical about this. Then again, someone doing copy-paste might copy only the file that applies to them and skip support for a compiler they don't use anyway. Either way, if I would be doing it today I might very well go into one file only, skipping also the CMake part.

All this code seems more adequate for a blog entry kind of publication rather than a fully-fledged library. I expect that even if someone would actually want to use this version they would anyway copy-paste the code and perhaps rework it to their liking (like rename following local convention) rather than download this and consider it an external dependency. And this is an argument for your approach.

I check for the semicolon and if not found, I look for the ending square bracket using "find()".

Do you properly handle ';' as a template argument? This is my main concern... Is your code to be seen online somewhere?

It's a wonder that in 2023, there's nothing in the standard that does this yet (returns a type's WYSIWYG name). Names returned by "type_info::name" aren't always clean, and it's also runtime only (in light of the movement of so much in C++ towards "constexpr" techniques in recent years).

Yeah! At least a constexpr version of typeid (that also wouldn't require RTTI!) seems like something we should have a long time ago.

BTW, I think the __PRETTY_FUNCTION__ can be used for more than just the type name. We should be able to retrieve namespaces, template arguments, and their values. Furthermore, there are publications that use it to explore enums, for example Enum reflection in C++ with template metaprogramming - the code there is somewhat lacking, I have slightly improved it in https://godbolt.org/z/KhsGndcPK, however, this approach is too consuming for the compiler, trying to cover large range will kill the compilation time.

HexadigmSystems commented 1 year ago

I wanted to make a "proper library

Well, a number of issues exist with the separate header approach in general but for something like "type_name" the code is trivial anyway so it's not a big deal. I wouldn't worry about it.

Do you properly handle ';' as a template argument? This is my main concern... Is your code to be seen online somewhere

Yes, it's handled. You'll be able to see the code in the same header here once I post it (part of the header for my "FunctionTraits" struct which is pretty much done for all intents and purposes but busy with some other things now so will post ASAP). I had to extract all this code from a large generic library however, one I've been writing for many years (for my own private work). My own "TypeName_v" template (I always use Pascal and/or Camel casing - hate all lower-case), originates from this library that is and since I wasn't about to post the entire library just to provide users with a "FunctionTraits" struct (which also uses "TypeName_v"), I had to modify some of the code to eliminate these dependencies (including "TypeName_v"). The final code in the above link therefore won't be quite as small and clean as the one in my actual library because that one uses some of the other useful features in the library to carry out its work. Nevertheless, you'll be able to take a look at "TypeName_v" even though it's a bit more verbose than the one I actually use in my library (though both written to eliminate any dependencies on the function name such as its return type, as we've been discussing, so inherently a bit longer by necessity). Anyway, I'll post back here once I release my "FunctionTraits" struct.

HexadigmSystems commented 1 year ago

Forgot to mention this BTW (regarding your comment about enums):

https://github.com/Neargye/magic_enum

Still, why isn't the equivalent of this among other traits in the standard yet (forcing you and I many others to write this stuff)

HexadigmSystems commented 1 year ago

Finally got around to posting my FunctionTraits struct though may be tinkering with it a bit still. It includes my own "TypeName_v" variable template however (I prefer Pascal and Camel case) so see this if you're interested in my own implementation (declared in "TypeTraits.h" - implementation code not too long if your editor can hide the many comments). A demo can be found here but it's the same code in the link above (the link above will always contain the most up-to-date code though of course). The demo is for "FunctionTraits" itself (demo coded in "Demo.cpp") but it (ultimately) displays the sample function args seen in the output as strings using "TypeName_v" (or just download the code and play with it yourself - "TypeTraits.h" and "CompilerVersions.h" is all you need to use "TypeName_v"- #include the former which automatically #includes the latter, and you can then call "TypeName_v" in namespace "StdExt")