Removed mentions of Wdeclaration-after-statement now that C99 is requ…

[seanm]

…ired

[qkoziol]

Sorry, I misunderstood this change - I don't think that allowing declarations in the middle of a block is helpful for understanding how the code is structured and operates.

[derobins]

MSVC has historically been poor about this. I'm skeptical that code readability would improve with variable declarations scattered around functions (I feel the real solution is to break long functions into smaller ones), but this would be handy for loop variable declarations and removing the need for block brackets when we declare variables in ifdef'd code.

[seanm]

All I'm really proposing here is not to treat this warning as an error.

You can continue to put most variables at the beginning of a function by convention, but at least going forward cases like loop counters (as @derobins mentioned) can have reduced scope.

[qkoziol]

All I'm really proposing here is not to treat this warning as an error.

You can continue to put most variables at the beginning of a function by convention, but at least going forward cases like loop counters (as @derobins mentioned) can have reduced scope.

Yes, I understand, but I still think everything except loop variables should be an error, so they don't creep in.

[seanm]

but I still think everything except loop variables should be an error

I don't think there's any way to do that. (I also totally disagree :) but won't argue that point. :))

[byrnHDF]

Just a side comment about preferring declarations at the top based on experiences. Searching through a long function with declarations sprinkled about makes it harder to fully understand. Of course shorter functions help with both issues.
But logical groupings help too.
A second issue much more important to me is that we still have to support three other branches and merges are already difficult enough with worrying about features that need to be worked around.
Conclusion; requiring declarations at the top are a minor annoyance for not much improvement given the capability of todays compilers.

[gnuoyd]

Mid-block declarations are a tool that I use to clarify code, especially long functions, especially when I'm preparing to make those long functions shorter.

Sometimes I use mid-block declarations to enforce single assignment on a variable, either because that makes my intent crystal clear, or because I am refactoring and I want to poison subsequent assignments, using the errors spit by the compiler to find them.

@byrnHDF , are we supporting the three other branches (which are these? 1.8, 1.10, 1.12?) on different compilers than develop? It seems to me that we have already committed to C99. Please elaborate on your concern.

When you say functions with mid-block declarations are "hard to understand," I am skeptical. Hard to understand how? Why?

I don't see why this should not be left to a programmer's discretion. If functions come out hard to understand, then we'll catch that during review.

[byrnHDF]

The comments about mid-functions were in relation to other code bases, where there was probably not a rigorous review process.
The comment about merges to other branches are from many battles merging a innocuous change to the other branches because of changing code conventions. If you do it enough you are willing to sacrifice some freedom for simpler merges!

[derobins]

@byrnHDF , are we supporting the three other branches (which are these? 1.8, 1.10, 1.12?) on different compilers than develop? It seems to me that we have already committed to C99. Please elaborate on your concern.

Microsoft's support for C99 (or any C standard, really) has historically been atrocious. Even though we've agreed that we require C99, we still have to hedge that when it comes to Microsoft as we have a lot of users on that platform who build with that compiler. Heck, even the C standard library isn't fundamentally a Windows thing.

When using a C99 feature that is not already in use in the HDF5 codebase, you usually have to check to see if MSVC is going to have problems. Now that we've moved our earliest supported VS version to 2015, this should be less of an issue, but it's still something to be wary of. Ideally, we'd check for this via a github action, but the earliest version of VS they offer is 2017. We switched one of the actions to build against that, which will help, but 2015 checks are only done via our internal CI, which isn't as visible when it fails.

[derobins]

As for maintaining multiple branches, that's probably a lower-order concern here. If anything, not having a block of top-level declarations might make merges less annoying. Merge algorithms have problems figuring out what to do when two changes tail append to the same chunk of code, and that includes the variable declaration blocks.

As an example, if I start with this:

int foo = 0;

And change it to this:

int foo = 0;
int bar = 0;

but somebody else merges a change to the parent branch that looks like this:

int foo = 0;
int baz = 0;

git will definitely complain when I pull in the upstream changes.

If that 'int baz = 0;' line were located near its use, the likelihood of a conflict would probably go down, particularly in longer functions. Again, the real solution is shorter functions, but in the meantime moving the declarations around could actually help when it comes to merging.

[derobins]

It looks like VS2013 finally allowed declarations after statements as well as C99 Boolean variables. Now that VS2010/2012 are in the rearview mirror, we should probably think about allowing declarations mixed with code and change hbool_t/TRUE/FALSE to bool/true/false (at least internally, since hbool_t is in the public API).

[derobins]

A lot of the crap in H5public.h can also probably go away, now that C99 integer types are ubiquitous. I have a branch with some of that work done as it's needed to get the internal bits of H5pubconf.h out of the public API, which is a personal pet peeve of mine.

[seanm]

It looks like VS2013 finally allowed declarations after statements as well as C99 Boolean variables

Yes, and according to this "Visual Studio 2015 fully implements the C99 Standard Library, with the exception of any library features that depend on compiler features not yet supported by the Visual C++ compiler (for example, <tgmath.h> is not implemented)."

A lot of the crap in H5public.h can also probably go away, now that C99 integer types are ubiquitous

I was going to take a crack at that too, but trying not to make too many PRs at once. :)

[derobins]

I was going to take a crack at that too, but trying not to make too many PRs at once. :)

Just keep in mind that removing symbols triggers binary compatibility report entries, which we try to avoid. It's okay to remove symbols from develop / 1.13, but we can't remove those symbols from 1.12 or 1.10 without angering people who consume our library.

If you are removing public symbols, try to split the changes into two PRs: one for internal things, where we effectively stop using the feature, and one for the public API changes. That helps us move the internal changes to the other supported branches.

As an example, check out what I did with the changes in #457, where I replace H5_HAVE_FORK with H5_HAVE_UNISTD_H but don't remove the CMake or Autotools checks. I'll do that in a future PR.

[byrnHDF]

I should have stated that, first, I am not arguing against the idea, just that changing the convention requires more thought and planning then just switching the flag off. What guidance will we provide devs so that we don't end up with spaghetti. We have a lot of configurations and platforms supported - have we checked known problem installations. What about support libraries, plugins, examples, vols, external tests, etc..

[qkoziol]

I should have stated that, first, I am not arguing against the idea, just that changing the convention requires more thought and planning then just switching the flag off. What guidance will we provide devs so that we don't end up with spaghetti. We have a lot of configurations and platforms supported - have we checked known problem installations. What about support libraries, plugins, examples, vols, external tests, etc..

Totally agree! I could see allowing variable declarations only for loop variable to start and building up guidance for other usage.

[byrnHDF]

Should this warning also be taken out of error-general file or downgraded to just a warning in that file?
Also, line 2 and 3 of this file should be reworded. Maybe something like:

HDF5 code should not trigger the following warnings, they are identified
as candidates for the compiler to treat them as errors:

[gnuoyd]

There should not be a noerror-general file or any other noerror- file.

The error- and noerror- files are identical under a simple search and replace that is in config/gnu-flags: if PROMOTE_ERRORS is set to no then every occurrence of -Werror= is transformed to a warning (-W). Looks like the CMake files can do essentially the same thing.

[gnuoyd]

"Silence is golden." Declarations after statements are allowed, so let's just delete -Wdeclaration-after-statement everywhere it occurs.

[byrnHDF]

There should not be a noerror-general file or any other noerror- file.

The error- and noerror- files are identical under a simple search and replace that is in config/gnu-flags: if PROMOTE_ERRORS is set to no then every occurrence of -Werror= is transformed to a warning (-W). Looks like the CMake files can do essentially the same thing.

But wouldn't that promote ALL warnings? I thought the error-general file was intended to only promote those that should pass?

[byrnHDF]

"Silence is golden." Declarations after statements are allowed, so let's just delete -Wdeclaration-after-statement everywhere it occurs.

I didn't think we came to a conclusion on this. We would need a way to identify (maybe a ./bin/check_declaration script?) what was valid vs not wanted behavior.

[gnuoyd]

I don't know what you mean, "promote ALL warnings." Not all warnings are in the error- files. The error- files are supposed to contain those warnings that should ordinarily stop the library compilation cold.

[gnuoyd]

If there is an objectionable declaration after statement, then the place to flag that is during PR review.

[qkoziol]

If there is an objectionable declaration after statement, then the place to flag that is during PR review.

I'd rather avoid many "noisy" PRs and just keep the error

[byrnHDF]

From a quick review of the platforms being reported in DT, it looks like no configuration is complaining about for loop declarations. This is looking at code that I know have these for loop declarations (blosc filter). Therefore I don't think we need to remove this warning at this time and still allow for loop declarations.
Let's start by implementing for loop declarations and then in a few months take the next step if we all agree?

[lrknox]

It was decided after engineering discussion to remove the warnings/errors for declaration-after-statement, and to allow them at the beginning of for loops and ifdef blocks. They are to be avoided otherwise and we will depend on code reviews to enforce that.

[qkoziol]

It was decided after engineering discussion to remove the warnings/errors for declaration-after-statement, and to allow them at the beginning of for loops and ifdef blocks. They are to be avoided otherwise and we will depend on code reviews to enforce that.

Where is this written down?

[lrknox]

I wrote it down here because there didn't seem to be anywhere else to put it.
2 questions:
Do you disagree with the statement?
Do you have a suggestion as to where to post?

[qkoziol]

I wrote it down here because there didn't seem to be anywhere else to put it.
2 questions:
Do you disagree with the statement?
Do you have a suggestion as to where to post?

I'm OK with the decision, but current+future people reviewing PRs for conforming to this decision aren't going to be consulting closed PRs for coding style guidelines. ;-)

Is there a coding style guide where it makes sense?

[epourmal]

I don't think it belongs to the "coding style"-type of document. Guidance should be added to the source file.

[lrknox]

The source files in this PR are two Warning list files, error or noerror, and INSTALL_warnings.txt in release_docs, but the notice to code reviewers and code writers applies to hundreds or possibly a thousand source files. Allen and I brainstormed a bit in this morning's engineering standup and we suggest adding a paragraph similar to the one at the start of this discussion to the bottom of the doc/code-conventions.md file.

[qkoziol]

The source files in this PR are two Warning list files, error or noerror, and INSTALL_warnings.txt in release_docs, but the notice to code reviewers and code writers applies to hundreds or possibly a thousand source files. Allen and I brainstormed a bit in this morning's engineering standup and we suggest adding a paragraph similar to the one at the start of this discussion to the bottom of the doc/code-conventions.md file.

That sounds like a good place for it. :-)