Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

More clang tidy #908

Merged
merged 8 commits into from
Aug 23, 2021
Merged

More clang tidy #908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
576a7d7
Pacify clang-analyzer-unix.cstring.NullArg
seanm Mar 14, 2021
8dfc483
Apply some bugprone-suspicious-string-compare
seanm Mar 14, 2021
960883b
Apply some readability-simplify-boolean-expr
seanm Mar 14, 2021
8482056
Apply some readability-make-member-function-const
seanm Mar 14, 2021
3901779
Apple some bugprone-macro-parentheses
seanm Mar 14, 2021
798f0ab
Changed an f suffix to L for `long double`
seanm Mar 14, 2021
e9e1e20
Applied some readability-uppercase-literal-suffix automatically
seanm Mar 14, 2021
a53bd95
Committing clang-format changes
github-actions[bot] Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions c++/src/H5DataType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,10 +338,7 @@ DataType::encode()
bool
DataType::hasBinaryDesc() const
{
if (encoded_buf != NULL)
return true;
else
return false;
return encoded_buf != NULL;
}

//--------------------------------------------------------------------------
Expand Down
5 changes: 1 addition & 4 deletions c++/src/H5IdComponent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,10 +432,7 @@ IdComponent::p_valid_id(const hid_t obj_id)
return false;

H5I_type_t id_type = H5Iget_type(obj_id);
if (id_type <= H5I_BADID || id_type >= H5I_NTYPES)
return false;
else
return true;
return (id_type > H5I_BADID && id_type < H5I_NTYPES);
}

// Notes about IdComponent::id
Expand Down
18 changes: 9 additions & 9 deletions c++/test/tattr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ test_attr_getname()

// Check for existence of attribute FATTR1_NAME
bool attr_exists = fid1.attrExists(FATTR1_NAME);
if (attr_exists == false)
if (!attr_exists)
throw InvalidActionException("H5File::attrExists", "Attribute should exist but does not");

// Open attribute
Expand Down Expand Up @@ -361,7 +361,7 @@ test_attr_getname()

// Check for existence of attribute
attr_exists = dataset.attrExists(ATTR1_NAME);
if (attr_exists == false)
if (!attr_exists)
throw InvalidActionException("H5File::attrExists", "Attribute should exist but does not");

// Open attribute
Expand Down Expand Up @@ -410,7 +410,7 @@ test_attr_rename()

// Check for existence of attribute
bool attr_exists = fid1.attrExists(FATTR1_NAME);
if (attr_exists == false)
if (!attr_exists)
throw InvalidActionException("H5File::attrExists", "Attribute should exist but does not");

// Change attribute name
Expand All @@ -436,7 +436,7 @@ test_attr_rename()

// Check for existence of attribute
attr_exists = dataset.attrExists(ATTR1_NAME);
if (attr_exists == false)
if (!attr_exists)
throw InvalidActionException("H5File::attrExists", "Attribute should exist but does not");

// Change attribute name
Expand Down Expand Up @@ -464,7 +464,7 @@ test_attr_rename()

// Check for existence of second attribute
attr_exists = dataset.attrExists(ATTR2_NAME);
if (attr_exists == false)
if (!attr_exists)
throw InvalidActionException("H5File::attrExists", "Attribute should exist but does not");

// Open the second attribute
Expand Down Expand Up @@ -492,7 +492,7 @@ test_attr_rename()

// Check for existence of attribute after renaming
attr_exists = dataset.attrExists(ATTR1_NAME);
if (attr_exists == false)
if (!attr_exists)
throw InvalidActionException("H5File::attrExists", "Attribute should exist but does not");

PASSED();
Expand Down Expand Up @@ -1681,13 +1681,13 @@ test_attr_exists()

// Check for existence of attribute
bool attr_exists = fid1.attrExists(ATTR1_FL_STR_NAME);
if (attr_exists == false)
if (!attr_exists)
throw InvalidActionException("H5File::attrExists",
"fid1, ATTR1_FL_STR_NAMEAttribute should exist but does not");

// Check for existence of attribute
attr_exists = fid1.attrExists(FATTR1_NAME);
if (attr_exists == false)
if (!attr_exists)
throw InvalidActionException("H5File::attrExists",
"fid1,FATTR2_NAMEAttribute should exist but does not");

Expand All @@ -1696,7 +1696,7 @@ test_attr_exists()

// Check for existence of attribute
attr_exists = group.attrExists(ATTR2_NAME);
if (attr_exists == false)
if (!attr_exists)
throw InvalidActionException("H5File::attrExists",
"group, ATTR2_NAMEAttribute should exist but does not");

Expand Down
2 changes: 1 addition & 1 deletion c++/test/ttypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ test_named()
}

// Check that it is committed.
if (itype.committed() == false)
if (!itype.committed())
cerr << "IntType::committed() returned false" << endl;

// We should not be able to modify a type after it has been committed.
Expand Down
25 changes: 11 additions & 14 deletions hl/c++/src/H5PacketTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,17 @@ PacketTable::~PacketTable()
* any trouble making or opening the packet table.
*/
bool
PacketTable::IsValid()
PacketTable::IsValid() const
{
if (H5PTis_valid(table_id) == 0)
return true;
else
return false;
return H5PTis_valid(table_id) == 0;
}

/* IsVariableLength
* Return 1 if this packet table uses variable-length datatype,
* and 0, otherwise. Returns -1 if the table is invalid (not open).
*/
int
PacketTable::IsVariableLength()
PacketTable::IsVariableLength() const
{
return H5PTis_varlen(table_id);
}
Expand All @@ -84,7 +81,7 @@ PacketTable::IsVariableLength()
* Sets the index to point to the first packet in the packet table
*/
void
PacketTable::ResetIndex()
PacketTable::ResetIndex() const
{
H5PTcreate_index(table_id);
}
Expand All @@ -94,7 +91,7 @@ PacketTable::ResetIndex()
* Returns 0 on success, negative on failure (if index is out of bounds)
*/
int
PacketTable::SetIndex(hsize_t index)
PacketTable::SetIndex(hsize_t index) const
{
return H5PTset_index(table_id, index);
}
Expand All @@ -104,7 +101,7 @@ PacketTable::SetIndex(hsize_t index)
* Returns 0 on success, negative on failure (if index is out of bounds)
*/
hsize_t
PacketTable::GetIndex(int &error)
PacketTable::GetIndex(int &error) const
{
hsize_t index;

Expand All @@ -121,7 +118,7 @@ PacketTable::GetIndex(int &error)
* error is set to negative.
*/
hsize_t
PacketTable::GetPacketCount(int &error)
PacketTable::GetPacketCount(int &error) const
{
hsize_t npackets;

Expand All @@ -133,7 +130,7 @@ PacketTable::GetPacketCount(int &error)
* Returns the identifier of the packet table
*/
hid_t
PacketTable::GetTableId()
PacketTable::GetTableId() const
{
return table_id;
}
Expand All @@ -145,7 +142,7 @@ PacketTable::GetTableId()
* the desired functionality cannot be performed via the packet table ID.
*/
hid_t
PacketTable::GetDatatype()
PacketTable::GetDatatype() const
{
return H5PTget_type(table_id);
}
Expand All @@ -157,7 +154,7 @@ PacketTable::GetDatatype()
* the desired functionality cannot be performed via the packet table ID.
*/
hid_t
PacketTable::GetDataset()
PacketTable::GetDataset() const
{
return H5PTget_dataset(table_id);
}
Expand All @@ -169,7 +166,7 @@ PacketTable::GetDataset()
* Returns 0 on success, negative on error.
*/
int
PacketTable::FreeBuff(size_t numStructs, hvl_t *buffer)
PacketTable::FreeBuff(size_t numStructs, hvl_t *buffer) const
{
return H5PTfree_vlen_buff(table_id, numStructs, buffer);
}
Expand Down
20 changes: 10 additions & 10 deletions hl/c++/src/H5PacketTable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,39 +54,39 @@ class H5_HLCPPDLL PacketTable {
* Use this after the constructor to ensure HDF did not have
* any trouble making or opening the packet table.
*/
bool IsValid();
bool IsValid() const;

/* IsVariableLength
* Return 1 if this packet table uses variable-length datatype,
* return 0 if it is Fixed Length. Returns -1 if the table is
* invalid (not open).
*/
int IsVariableLength();
int IsVariableLength() const;

/* ResetIndex
* Sets the "current packet" index to point to the first packet in the
* packet table
*/
void ResetIndex();
void ResetIndex() const;

/* SetIndex
* Sets the current packet to point to the packet specified by index.
* Returns 0 on success, negative on failure (if index is out of bounds)
*/
int SetIndex(hsize_t index);
int SetIndex(hsize_t index) const;

/* GetIndex
* Returns the position of the current packet.
* On failure, returns 0 and error is set to negative.
*/
hsize_t GetIndex(int &error);
hsize_t GetIndex(int &error) const;

/* GetPacketCount
* Returns the number of packets in the packet table. Error
* is set to 0 on success. On failure, returns 0 and
* error is set to negative.
*/
hsize_t GetPacketCount(int &error);
hsize_t GetPacketCount(int &error) const;

hsize_t
GetPacketCount()
Expand All @@ -98,31 +98,31 @@ class H5_HLCPPDLL PacketTable {
/* GetTableId
* Returns the identifier of the packet table.
*/
hid_t GetTableId();
hid_t GetTableId() const;

/* GetDatatype
* Returns the datatype identifier used by the packet table, on success,
* or FAIL, on failure.
* Note: it is best to avoid using this identifier in applications, unless
* the desired functionality cannot be performed via the packet table ID.
*/
hid_t GetDatatype();
hid_t GetDatatype() const;

/* GetDataset
* Returns the dataset identifier associated with the packet table, on
* success, or FAIL, on failure.
* Note: it is best to avoid using this identifier in applications, unless
* the desired functionality cannot be performed via the packet table ID.
*/
hid_t GetDataset();
hid_t GetDataset() const;

/* FreeBuff
* Frees the buffers created when variable-length packets are read.
* Takes the number of hvl_t structs to be freed and a pointer to their
* location in memory.
* Returns 0 on success, negative on error.
*/
int FreeBuff(size_t numStructs, hvl_t *buffer);
int FreeBuff(size_t numStructs, hvl_t *buffer) const;

protected:
hid_t table_id;
Expand Down
2 changes: 1 addition & 1 deletion hl/c++/test/ptableTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,7 @@ TestHDFFV_9758()

for (hsize_t i = 0; i < NUM_PACKETS; i++) {
s1[i].a = static_cast<int>(i);
s1[i].b = 1.0f * static_cast<float>(i * i);
s1[i].b = 1.0F * static_cast<float>(i * i);
s1[i].c = 1.0 / static_cast<double>(i + 1);
HDsprintf(s1[i].d, "string%" PRIuHSIZE "", i);
s1[i].e = static_cast<int>(100 + i);
Expand Down
8 changes: 4 additions & 4 deletions hl/examples/ex_table_01.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ main(void)
sizeof(dst_buf[0].pressure), sizeof(dst_buf[0].temperature)};

/* Define an array of Particles */
Particle p_data[NRECORDS] = {{"zero", 0, 0, 0.0f, 0.0}, {"one", 10, 10, 1.0f, 10.0},
{"two", 20, 20, 2.0f, 20.0}, {"three", 30, 30, 3.0f, 30.0},
{"four", 40, 40, 4.0f, 40.0}, {"five", 50, 50, 5.0f, 50.0},
{"six", 60, 60, 6.0f, 60.0}, {"seven", 70, 70, 7.0f, 70.0}};
Particle p_data[NRECORDS] = {{"zero", 0, 0, 0.0F, 0.0}, {"one", 10, 10, 1.0F, 10.0},
{"two", 20, 20, 2.0F, 20.0}, {"three", 30, 30, 3.0F, 30.0},
{"four", 40, 40, 4.0F, 40.0}, {"five", 50, 50, 5.0F, 50.0},
{"six", 60, 60, 6.0F, 60.0}, {"seven", 70, 70, 7.0F, 70.0}};

/* Define field information */
const char *field_names[NFIELDS] = {"Name", "Latitude", "Longitude", "Pressure", "Temperature"};
Expand Down
10 changes: 5 additions & 5 deletions hl/examples/ex_table_02.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ main(void)
Particle dst_buf[NRECORDS + NRECORDS_ADD];

/* Define an array of Particles */
Particle p_data[NRECORDS] = {{"zero", 0, 0, 0.0f, 0.0}, {"one", 10, 10, 1.0f, 10.0},
{"two", 20, 20, 2.0f, 20.0}, {"three", 30, 30, 3.0f, 30.0},
{"four", 40, 40, 4.0f, 40.0}, {"five", 50, 50, 5.0f, 50.0},
{"six", 60, 60, 6.0f, 60.0}, {"seven", 70, 70, 7.0f, 70.0}};
Particle p_data[NRECORDS] = {{"zero", 0, 0, 0.0F, 0.0}, {"one", 10, 10, 1.0F, 10.0},
{"two", 20, 20, 2.0F, 20.0}, {"three", 30, 30, 3.0F, 30.0},
{"four", 40, 40, 4.0F, 40.0}, {"five", 50, 50, 5.0F, 50.0},
{"six", 60, 60, 6.0F, 60.0}, {"seven", 70, 70, 7.0F, 70.0}};

/* Calculate the size and the offsets of our struct members in memory */
size_t dst_size = sizeof(Particle);
Expand All @@ -66,7 +66,7 @@ main(void)
int i;

/* Append particles */
Particle particle_in[NRECORDS_ADD] = {{"eight", 80, 80, 8.0f, 80.0}, {"nine", 90, 90, 9.0f, 90.0}};
Particle particle_in[NRECORDS_ADD] = {{"eight", 80, 80, 8.0F, 80.0}, {"nine", 90, 90, 9.0F, 90.0}};

/* Initialize the field field_type */
string_type = H5Tcopy(H5T_C_S1);
Expand Down
6 changes: 3 additions & 3 deletions hl/examples/ex_table_03.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,14 @@ main(void)
size_t dst_offset[NFIELDS] = {HOFFSET(Particle, name), HOFFSET(Particle, lati), HOFFSET(Particle, longi),
HOFFSET(Particle, pressure), HOFFSET(Particle, temperature)};

Particle p = {"zero", 0, 0, 0.0f, 0.0};
Particle p = {"zero", 0, 0, 0.0F, 0.0};
size_t dst_sizes[NFIELDS] = {sizeof(p.name), sizeof(p.lati), sizeof(p.longi), sizeof(p.pressure),
sizeof(p.temperature)};

/* Define field information */
const char *field_names[NFIELDS] = {"Name", "Latitude", "Longitude", "Pressure", "Temperature"};
/* Fill value particle */
Particle fill_data[1] = {{"no data", -1, -1, -99.0f, -99.0}};
Particle fill_data[1] = {{"no data", -1, -1, -99.0F, -99.0}};
hid_t field_type[NFIELDS];
hid_t string_type;
hid_t file_id;
Expand All @@ -63,7 +63,7 @@ main(void)
int i;

/* Define 2 new particles to write */
Particle particle_in[NRECORDS_WRITE] = {{"zero", 0, 0, 0.0f, 0.0}, {"one", 10, 10, 1.0f, 10.0}};
Particle particle_in[NRECORDS_WRITE] = {{"zero", 0, 0, 0.0F, 0.0}, {"one", 10, 10, 1.0F, 10.0}};

/* Initialize the field field_type */
string_type = H5Tcopy(H5T_C_S1);
Expand Down
10 changes: 5 additions & 5 deletions hl/examples/ex_table_04.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,23 +65,23 @@ main(void)
hid_t string_type;
hid_t file_id;
hsize_t chunk_size = 10;
Particle fill_data[1] = {{"no data", -1, -1, -99.0f, -99.0}}; /* Fill value particle */
Particle fill_data[1] = {{"no data", -1, -1, -99.0F, -99.0}}; /* Fill value particle */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we just change all these to lower case "f" earlier this year?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I don't follow HDF5 closely. The clang-tidy docs are here but they don't really give a rationale. I think it's mostly for l which can look a lot like 1. I guess the idea is to consistently use uppercase. I can drop this commit, I don't have strong feelings about it for f vs F, but for l vs L I do think uppercase is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure myself, but I think it was a gcc warning issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just checked the develop branch, and there is a mix of f and F currently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsurprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So using readability-uppercase-literal-suffix as per this patch would have the advantage of making it uniform, even though F vs f is not as useful as l vs L.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did a quick check and it seems we have no standard for F vs f. We will need to decide so we don't keep churning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have no standard, but I'm fine with upper case F being the standard, especially given the l vs L confusion.

hsize_t start; /* Record to start reading/writing */
hsize_t nrecords; /* Number of records to read/write */
int compress = 0;
int i;
Particle *p_data = NULL; /* Initially no data */
float pressure_in[NRECORDS_ADD] = /* Define new values for the field "Pressure" */
{0.0f, 1.0f, 2.0f};
{0.0F, 1.0F, 2.0F};
Position position_in[NRECORDS_ADD] = {/* Define new values for "Latitude,Longitude" */
{0, 0},
{10, 10},
{20, 20}};
NamePressure namepre_in[NRECORDS_ADD] = /* Define new values for "Name,Pressure" */
{
{"zero", 0.0f},
{"one", 1.0f},
{"two", 2.0f},
{"zero", 0.0F},
{"one", 1.0F},
{"two", 2.0F},
};
size_t field_sizes_pos[2] = {sizeof(position_in[0].longi), sizeof(position_in[0].lati)};
size_t field_sizes_pre[1] = {sizeof(namepre_in[0].pressure)};
Expand Down
Loading