Details
- Reviewers
ldionne Mordante var-const - Group Reviewers
Restricted Project - Commits
- rG4887d047a31f: [libc++][NFC] Replace enable_if with __enable_if_t in a few places
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
In general I'm happy. But please avoid unrelated NFC code changes is these kind of commits. Especially since the title nor the description mentions them. (Thus some minor whitespace fixes are not an issue.)
libcxx/include/__split_buffer | ||
---|---|---|
121 | I'm not happy with this change in this commit, please make a separate review for it. |
This isn't quite an NFC I think. Specifically, __enable_if_t is marked with _LIBCPP_NODEBUG whereas enable_if is not. The new behavior proposed in this patch results in suppressing debugging information for these functions. Does anyone rely on this behavior/care?
IIUC this only suppresses debug information of the __enable_if_t itself. It doesn't suppress any other debug information. So Instead of the function debug info being enable_if<some stuff>::type you get just void. So yes, it technically changes something, but I don't think anybody really cares for this part of the debug info.
LGTM with comments applied.
libcxx/include/__tree | ||
---|---|---|
1285 | This (and line 1295) makes the indentation inconsistent in the file. Please keep the 4 space indentation for consistency. | |
libcxx/include/array | ||
435 | Here and in other return types where you did the same transformation, please keep the explicit void type at the end. I know it's redundant, but it helps to see what is the intended return type of the function. We should also refactor these enable_ifs to use template parameters instead. |
I'm not happy with this change in this commit, please make a separate review for it.