Skip to content

Comments

Fix syntax inside switch#3365

Merged
shelhamer merged 1 commit intoBVLC:masterfrom
BonsaiAI:wrap-declarations-in-switch
Jan 19, 2017
Merged

Fix syntax inside switch#3365
shelhamer merged 1 commit intoBVLC:masterfrom
BonsaiAI:wrap-declarations-in-switch

Conversation

@eelstork
Copy link
Contributor

glog macros inside switch cause the MSVC compiler to emit error C2360.
I find that GCC may be lenient in this case; standards compliance should we require that we manually define scopes to declare and initialise variables within case statements.
Additionally, LOG(FATAL) doesn't exonerate us from the requirement to break after case.

@lukeyeager
Copy link
Contributor

See #3130 as well.

@longjon
Copy link
Contributor

longjon commented Nov 22, 2015

Note that, by my quick read of https://github.com/google/glog/blob/master/src/glog/logging.h.in, it looks like this issue only affects LOG_FIRST_N.

This feels like an upstream issue to me; in general, macros should behave like function calls. We could consider a workaround (e.g., a wrapper around LOG_FIRST_N) pending an upstream fix (i.e., patch this in glog), but I'm not a big fan of mysteriously augmenting lots of calls that look perfectly fine.

Additionally, LOG(FATAL) doesn't exonerate us from the requirement to break after case.

Sure it does, just as return or exit(0) does.

@eelstork
Copy link
Contributor Author

Raising an issue and suggesting a fix to the glog team.
In the meantime, I have updated the proposed fix to reflect suggested improvements.

@eelstork eelstork closed this Nov 23, 2015
@eelstork eelstork reopened this Nov 23, 2015
@eelstork
Copy link
Contributor Author

(closed by mistake; I wouldn't close this since I don't know whether it will be fixed in glog)

@eelstork eelstork closed this Nov 23, 2015
@eelstork eelstork reopened this Nov 23, 2015
@eelstork
Copy link
Contributor Author

With reference to google/glog#71 and google/glog#72, it appears that this can't be fixed in glog. @longjon, would you consider merging this?

@shelhamer
Copy link
Member

@willyd is this patch still helpful for windows, and if so is #3130 even better for compatibility?

@eelstork
Copy link
Contributor Author

Summary of what I know: patch is helpful; not fixable as an upstream issue (we tried); @longjon already debunked superfluous changes in #3130

@willyd
Copy link
Contributor

willyd commented Jan 19, 2017

@shelhamer I agree with @eelstork the patch is needed for windows compatibility. The current state of the windows branch looks more like #3130 but this PR has only the required changes so if it was my call I would merge this one and close #3130 without merging it. Current windows branch also as a comment explaining the new scope creation see this comment.

@shelhamer shelhamer merged commit c86433a into BVLC:master Jan 19, 2017
@shelhamer
Copy link
Member

Thanks @eelstork for the fix, @longjon for the investigation, and @willyd for the current branch perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants