Update reformatting settings, trace wrapping, and printf-format strings #22
Conversation
comments for code that performs this operation.
qkoziol
requested review from
byrnHDF,
derobins,
fortnern,
jhendersonHDF,
lrknox,
soumagne,
vchoi-hdfgroup,
bmribler and
jrmainzer
as code owners
|
The hsize_t/hssize_t typedefs should not be tied to a particular # of bits, they just need to be at least that many bits, and I updated the PRI*HSIZE macros to match. |
| @@ -5,14 +5,27 @@ AlignConsecutiveMacros: true | |||
| AlignConsecutiveAssignments: true | |||
| AlignConsecutiveDeclarations: true | |||
| AlwaysBreakAfterReturnType: AllDefinitions | |||
| # Can enable the following section when llvm 12.x is out | |||
There was a problem hiding this comment.
We should keep track of the versions somehow (There were some version 11 changes that would be nice).
It will likely depend on the version of clang on the ubuntu runner provided github.
There was a problem hiding this comment.
We should probably update the format_source script to detect the version of clang-format that's installed and use the correct formatting file. And maybe set a minimum version to use, so that our formatting is stable.
|
On Mon, Oct 05, 2020 at 08:01:15PM -0700, Quincey Koziol wrote:
The hsize_t/hssize_t typedefs should not be tied to a particular # of bits, they just need to be at least that many bits and I corrected the PRIdHSIZE macro.
If 64 bits is sufficient to express object sizes, but hsize_t is
larger than 64 bits---128 or 256 bits, let us say---then the hsize_t
storage size doubles or quadruples. The range of values we can
use is still 0..2^64-1, but the library's cache footprint increases,
which has real downsides. What's the upside?
Dave
|
|
The "upside" is source / binary / file format compatibility, as hsize_t is not specified as a particular size and tying it to one will create dependencies on this size in the future. |
|
On Tue, Oct 06, 2020 at 08:41:18AM -0700, Quincey Koziol wrote:
The "upside" is application binary compatibility.
That's a reasonable justification for changing it back to `long long`
until there is an ABI version bump.
Dave
|
|
OK, please approve so I can merge this. (Although we'll have to discuss other changes to the typedef in the future) |
|
OK, I'll update the clang-format-check.yml file, thanks! |
|
I've updated the clang-format scripts and the github actions to avoid processing generated files. As long as the checks pass, I believe that this is ready to merge. |
|
Had to post-process the H5LTparse.h header also, for newer versions of bison... |
|
@byrnHDF - Sure, I can update that file, but where / how is it invoked? I need to add another parameter to it. |
|
I am having trouble finding it. I will have to do it, I guess.
No problem, it was just a way for CMake to do the same thing.
But could you do the LTanalyze.l change - the diags should be in genparser not in the .l file.
Also notice I blocked the GNUC define because there are two different versions.
If what I did is wrong I'll do it later. It's just that I was having repo issues and am rebuilding my clones.
Allen
I will take off the request block.
…________________________________________
From: Quincey Koziol <[email protected]>
Sent: Tuesday, October 6, 2020 4:10 PM
To: HDFGroup/hdf5
Cc: Allen Byrne; Mention
Subject: Re: [HDFGroup/hdf5] Update reformatting settings, trace wrapping, and printf-format strings (#22)
@byrnHDF<https://github.com/byrnHDF> - Sure, I can update that file, but where / how is it invoked? I need to add another parameter to it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#22 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AL77KBQHWPKZZ4XUGXKFOZLSJOBTNANCNFSM4SFIX5SQ>.
|
|
the yml changes have an issue:
Check failure on line 18 in .github/workflows/clang-format-check.yml
GitHub Actions
/ clang-format Check
Invalid workflow file
The workflow is not valid. .github/workflows/clang-format-check.yml (Line: 18, Col: 9): A sequence was not expected
…________________________________________
From: Allen Byrne <[email protected]>
Sent: Tuesday, October 6, 2020 4:23 PM
To: HDFGroup/hdf5; HDFGroup/hdf5
Cc: Mention
Subject: Re: [HDFGroup/hdf5] Update reformatting settings, trace wrapping, and printf-format strings (#22)
I am having trouble finding it. I will have to do it, I guess.
No problem, it was just a way for CMake to do the same thing.
But could you do the LTanalyze.l change - the diags should be in genparser not in the .l file.
Also notice I blocked the GNUC define because there are two different versions.
If what I did is wrong I'll do it later. It's just that I was having repo issues and am rebuilding my clones.
Allen
I will take off the request block.
________________________________________
From: Quincey Koziol <[email protected]>
Sent: Tuesday, October 6, 2020 4:10 PM
To: HDFGroup/hdf5
Cc: Allen Byrne; Mention
Subject: Re: [HDFGroup/hdf5] Update reformatting settings, trace wrapping, and printf-format strings (#22)
@byrnHDF<https://github.com/byrnHDF> - Sure, I can update that file, but where / how is it invoked? I need to add another parameter to it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#22 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AL77KBQHWPKZZ4XUGXKFOZLSJOBTNANCNFSM4SFIX5SQ>.
|
|
Multiple exclude rules can also be put in a .clang-format-ignore file, which also supports comments.
Example:
# ignore third_party code from clang-format checks
src/third_party/*
src/ignored/*
…________________________________________
From: Allen Byrne <[email protected]>
Sent: Tuesday, October 6, 2020 4:25 PM
To: HDFGroup/hdf5; HDFGroup/hdf5
Cc: Mention
Subject: Re: [HDFGroup/hdf5] Update reformatting settings, trace wrapping, and printf-format strings (#22)
the yml changes have an issue:
Check failure on line 18 in .github/workflows/clang-format-check.yml
GitHub Actions
/ clang-format Check
Invalid workflow file
The workflow is not valid. .github/workflows/clang-format-check.yml (Line: 18, Col: 9): A sequence was not expected
________________________________________
From: Allen Byrne <[email protected]>
Sent: Tuesday, October 6, 2020 4:23 PM
To: HDFGroup/hdf5; HDFGroup/hdf5
Cc: Mention
Subject: Re: [HDFGroup/hdf5] Update reformatting settings, trace wrapping, and printf-format strings (#22)
I am having trouble finding it. I will have to do it, I guess.
No problem, it was just a way for CMake to do the same thing.
But could you do the LTanalyze.l change - the diags should be in genparser not in the .l file.
Also notice I blocked the GNUC define because there are two different versions.
If what I did is wrong I'll do it later. It's just that I was having repo issues and am rebuilding my clones.
Allen
I will take off the request block.
________________________________________
From: Quincey Koziol <[email protected]>
Sent: Tuesday, October 6, 2020 4:10 PM
To: HDFGroup/hdf5
Cc: Allen Byrne; Mention
Subject: Re: [HDFGroup/hdf5] Update reformatting settings, trace wrapping, and printf-format strings (#22)
@byrnHDF<https://github.com/byrnHDF> - Sure, I can update that file, but where / how is it invoked? I need to add another parameter to it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#22 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AL77KBQHWPKZZ4XUGXKFOZLSJOBTNANCNFSM4SFIX5SQ>.
|
|
@byrnHDF - I didn't change the .l file or your change to the genparser script, so I'm confused about what you are thinking of there. And, I like the idea of using the .clang-format-ignore file instead of the complicated scripts, etc, so I'll switch to that method, and update the PR (again :-) ). |
|
Wow, talk about opening a can of worms! Finally there is a version 0.10 of the clang-format-check module see line 11: |
|
OK, the .clang-format-ignore is not going to work well, since it's specific to the clang-format workflow on github and I'd have to make another copy of the listed files. So, I just updated the yml file. |
|
OK, I'll update those too. ;-) |
|
@byrnHDF - I updated the github clang-format action version, but there's no pragmas in the H5LTanalyze.l file. |
|
Okay good. Maybe I had already did that accidentally in a previous PR? |
Remove incorrect trailing '{' in src/H5C. that was confusing clang-format, align bin/trace with the line wrap limit that clang-format is using, update BEGIN_FUNC / END_FUNC macro settings for clang-format (for the H5EA*, H5FA*, and H5HL* files), and clean up and correct printf-format changes a little.