Fix /W4 level warnings in MSVC++ 2015 #41
Conversation
Remove unused variable names and use underlying class for enum (VS 2012 and above support strongly typed enums and will warn about incorrect usage).
enum.h
Outdated
| @@ -349,7 +351,7 @@ BETTER_ENUMS_CONSTEXPR_ static T* _or_null(optional<T*> maybe) | |||
|
|
|||
| template <typename T, typename U> | |||
| BETTER_ENUMS_CONSTEXPR_ U | |||
| continue_with(T ignored BETTER_ENUMS_UNUSED, U value) { return value; } | |||
| continue_with(T BETTER_ENUMS_UNUSED_WRAPPER(ignored) BETTER_ENUMS_UNUSED, U value) { return value; } | |||
There was a problem hiding this comment.
Why not simply remove completely the parameter name:
continue_with(T, U value) { return value; }
There was a problem hiding this comment.
Alternative PR is welcome. I didn't merge this mainly because of lack of time to port this PR to clang (see Travis failures), easy as that should be :/
There was a problem hiding this comment.
Or perhaps we should first wait for @Falconne to respond/amend :)
There was a problem hiding this comment.
I should probably apologize; I intended to do the port myself real quick, and then merge, which is why I didn't leave a comment about that. But then, I got a ton of work to do elsewhere, and neglected to update that I don't have the time anymore, for now...
There was a problem hiding this comment.
I didn't realise there was a failure in the clang build, I have pushed a fix that should hopefully address that.
I hadn't removed the unused variable because I thought there was a reason it was left there to begin with; I thought maybe GCC needed it. Was there a reason the BETTER_ENUMS_UNUSED macro was created in the first place instead of the variables simply being omitted?
Anyway, I've also removed the unused variable name in the commit and pushed that as well, I'll see how the build goes. Should I remove the used variable definition in the operator overload below as well?
There was a problem hiding this comment.
IIRC, I simply forgot about the option of not giving names. I welcome removing all such instances and the ..._UNUSED macro, it sounds like a strict improvement.
There was a problem hiding this comment.
I screwed up the MSC_VER check in the last push, hence the new build errors. I'll fix that later today and do another push. I'll remove the unused variable in the operator overload too.
There was a problem hiding this comment.
No problem; there is no rush. Obviously, I wasn't rushing (sorry), so it's only fair if you take your time :)
There was a problem hiding this comment.
I think I have it right now, but two of the Travis builds are failing due to what seems to be environmental issues on the build agent. I'm not sure what to do about that.
|
I restarted the builds. Let’s see what happens..
…
@Falconne commented on this pull request.
In enum.h <#41 (comment)>:
> @@ -349,7 +351,7 @@ BETTER_ENUMS_CONSTEXPR_ static T* _or_null(optional<T*> maybe)
template <typename T, typename U>
BETTER_ENUMS_CONSTEXPR_ U
-continue_with(T ignored BETTER_ENUMS_UNUSED, U value) { return value; }
+continue_with(T BETTER_ENUMS_UNUSED_WRAPPER(ignored) BETTER_ENUMS_UNUSED, U value) { return value; }
I think I have it right now, but two of the Travis builds are failing due to what seems to be environmental issues on the build agent. I'm not sure what to do about that.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALg6xM3QjWUzHy6FD07mRINiG_naLHBtks5sJY4dgaJpZM4M4Zfi>.
<https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png> <https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png> <https://github.com/aantron/better-enums> <#41 (comment)>
|
|
Ok, looking better now :) I want to squash-merge this PR. The commit message will be the same as in the first commit (but GitHub will add the PR number to it). If that's okay with you, you don't have to do anything more – thanks! |
|
Yep. I didn't do it myself because I didn't know if it would affect the pull request. So yeah, squash it when you merge. |
|
No worries. I actually prefer this way (now that GitHub supports it after years of requesting :)). We get to keep the actual history of work in this PR, while I check to make sure it's okay with you, since I'm doing something with sentences that you wrote :) Thanks for taking care of this! |
Remove unused variable names and use underlying class for enum (VS 2012 and above support strongly typed enums and will warn about incorrect usage).