Fix MISRA rule 8-3(1/2) 'declarations of a function same name and type' #1742

Splinter1984 · 2023-06-28T08:34:02Z

This PR addresses rule 8.3.

The identifiers used in the declaration and definition of a function shall be identical.

src/core/ddsc/src/dds_publisher.c

src/core/ddsc/src/dds_reader.c

eboasson

Thanks @Splinter1984. In some of the cases I think you picked the wrong name to use consistently. The vast majority is fine, though.

eboasson · 2023-06-30T12:48:14Z

src/core/ddsc/include/dds/ddsc/dds_rhc.h

@@ -63,13 +63,13 @@ DDS_INLINE_EXPORT inline dds_return_t dds_rhc_associate (struct dds_rhc *rhc, st
 }

 /** @component rhc */
-DDS_INLINE_EXPORT inline bool dds_rhc_store (struct dds_rhc * __restrict rhc, const struct ddsi_writer_info * __restrict wrinfo, struct ddsi_serdata * __restrict sample, struct ddsi_tkmap_instance * __restrict tk) {
-  return rhc->common.ops->rhc_ops.store (&rhc->common.rhc, wrinfo, sample, tk);
+DDS_INLINE_EXPORT inline bool dds_rhc_store (struct dds_rhc * __restrict rhc, const struct ddsi_writer_info * __restrict pwr_info, struct ddsi_serdata * __restrict sample, struct ddsi_tkmap_instance * __restrict tk) {


It would be better to consistently use wrinfo (or wr_info) here: the code base consistently uses wr for a writer and pwr for a proxy writer (and similarly, rd/prd for readers and proxy readers). So to have a type named struct ddsi_writer_info and then call the parameter pwr_info is inconsistent.¹

As a historical aside: in its very first incarnation (about a decade ago) this only dealt with remote data and hence it was always "proxy writer info" and apparently some parameter names stuck to that when local writers got added, even if the type names got fixed.

Footnotes

One immediately sees the futility of MISRA! ↩

eboasson · 2023-06-30T12:50:04Z

src/core/ddsc/src/dds_entity.c

@@ -1735,11 +1735,11 @@ static void pushdown_write_flush (dds_entity *e)
  ddsrt_mutex_unlock (&e->m_mutex);
 }

-dds_return_t dds_write_flush (dds_entity_t entity)
+dds_return_t dds_write_flush (dds_entity_t writer)


Here entity really is better: it can be a writer, publisher, participant or domain. I suppose that means the declaration in dds.h needs to be updated instead.

This is what happens when you blindly follow the MISRA rules!

eboasson · 2023-06-30T12:52:31Z

src/core/ddsc/src/dds_guardcond.c

@@ -31,7 +31,7 @@ const struct dds_entity_deriver dds_entity_deriver_guardcondition = {
  .refresh_statistics = dds_entity_deriver_dummy_refresh_statistics
 };

-dds_entity_t dds_create_guardcondition (dds_entity_t owner)
+dds_entity_t dds_create_guardcondition (dds_entity_t participant)


Here it is similar, the "owner" of the guard condition can be a participant, domain or even the root of the entity hierarchy. That makes participant a somewhat confusing name.

(As a historical aside: originally I thought it would be possible to limit it to participants. That turned out to be a wrong a call ... but like in some other cases, the declaration in dds.h wasn't updated along with the implementation.)

owner got back.

eboasson · 2023-06-30T12:54:46Z

src/core/ddsc/src/dds_reader.c

@@ -372,9 +372,9 @@ STATUS_CB_IMPL (reader, sample_lost,                SAMPLE_LOST,
 STATUS_CB_IMPL (reader, requested_deadline_missed,  REQUESTED_DEADLINE_MISSED,  total_count_change)
 STATUS_CB_IMPL (reader, requested_incompatible_qos, REQUESTED_INCOMPATIBLE_QOS, total_count_change)

-void dds_reader_status_cb (void *ventity, const ddsi_status_cb_data_t *data)


I use this v prefix quite often for these void * generic arguments ... in this case it gets cast to a dds_reader *rd, so it doesn't matter so much, but I'd like to keep it. Does that mean all those function type definitions need to be changed? I guess so ...

I don't know why I decided to change it here 😐.

eboasson · 2023-06-30T12:57:33Z

src/core/ddsc/src/dds_reader.c

@@ -758,11 +758,11 @@ dds_entity_t dds_create_reader_rhc (dds_entity_t participant_or_subscriber, dds_
  return dds_create_reader_int (participant_or_subscriber, topic, qos, listener, rhc);
 }

-uint32_t dds_reader_lock_samples (dds_entity_t reader)
+uint32_t dds_reader_lock_samples (dds_entity_t entity)


So the function type uses entity because it is an entity in general, but these are specialised for a specific entity type ... so now MISRA is forcing my poor brain to track that where it says entity it really means reader ... I thought MISRA was all about reducing the risk of confusion!

(But then they have the "single exit" rule, which every programmer has learnt only increases the state to be tracked when reading the code and hence increases the likelihood of mistakes. I should try to find out who came up with that rule, because I think it is actually older than MISRA, probably some study of common coding mistakes ...)

eboasson · 2023-06-30T13:05:38Z

src/core/ddsc/include/dds/ddsc/dds_public_status.h

@@ -306,7 +306,7 @@ dds_get_liveliness_lost_status (
 */
 DDS_EXPORT dds_return_t
 dds_get_offered_deadline_missed_status(
-  dds_entity_t writer,


This really only takes a writer, so I think "writer" is the better choice.

Applied and fixed.

eboasson · 2023-06-30T13:05:42Z

src/core/ddsc/include/dds/ddsc/dds_public_status.h

@@ -277,7 +277,7 @@ dds_get_publication_matched_status (
 */
 DDS_EXPORT dds_return_t
 dds_get_liveliness_lost_status (
-  dds_entity_t writer,


This really only takes a writer, so I think "writer" is the better choice.

Applied and fixed.

eboasson · 2023-06-30T13:05:48Z

src/core/ddsc/include/dds/ddsc/dds_public_status.h

@@ -248,7 +248,7 @@ dds_get_inconsistent_topic_status (
 */
 DDS_EXPORT dds_return_t
 dds_get_publication_matched_status (
-  dds_entity_t writer,


This really only takes a writer, so I think "writer" is the better choice.

Applied and fixed.

eboasson · 2023-06-30T13:05:57Z

src/core/ddsc/include/dds/ddsc/dds_public_status.h

@@ -219,7 +219,7 @@ dds_inconsistent_topic_status_t;
 */
 DDS_EXPORT dds_return_t
 dds_get_inconsistent_topic_status (
-  dds_entity_t topic,


This really only takes a topic, so I think "topic" is the better choice.

MISRA want the same argument names for all dds__get_status functions.

cyclonedds/src/core/ddsc/src/dds__get_status.h

Lines 23 to 35 in 93ff10f

#define DDS_GET_STATUS_COMMON(ent_type_, status_) \

dds_return_t dds_get_##status_##_status (dds_entity_t entity, dds_##status_##_status_t *status) \

{ \

dds_##ent_type_ *ent; \

dds_return_t ret; \

if ((ret = dds_##ent_type_##_lock (entity, &ent)) != DDS_RETCODE_OK) \

return ret; \

ddsrt_mutex_lock (&ent->m_entity.m_observers_lock); \

dds_get_##status_##_status_locked (ent, status); \

ddsrt_mutex_unlock (&ent->m_entity.m_observers_lock); \

dds_##ent_type_##_unlock (ent); \

return DDS_RETCODE_OK; \

}

Ah, of course ...

Maybe we can solve it by using ent_type_ for the parameter name, though. Like:

dds_return_t dds_get_##status_##_status (dds_entity_t ent_type_, dds_##status_##_status_t *status)

I haven't tried it, but I think it should work, because that's the bit in the type name after the dds_ prefix, so reader, writer, topic, &c.

This whole macro violates MISRA rules 20.10, but that's only advisory so it doesn't matter 🙂

Yeah, thank you it works 😎.

eboasson · 2023-06-30T13:06:35Z

src/core/ddsc/include/dds/ddsc/dds_internal_api.h

 *
 * @returns the number of samples in the reader cache.
 */
 DDS_EXPORT uint32_t
-dds_reader_lock_samples (dds_entity_t reader);


This really only takes a reader, so I think "reader" is the better choice. But see #1744. It is finally on its way out, so I don't care about this one! 🥳

I care! (That's why fixed)

eboasson

Thanks @Splinter1984 !

Splinter1984 added 4 commits June 28, 2023 10:30

Fix MISRA rule 8-3(1/2) 'declarations of a function same name and type'

0df4d0a

resolve conflicts with master

1434f38

resolve unknown type error

c0998c9

resolve ddsi_status_cb_data_t unknown type error

cb459cd

github-advanced-security bot found potential problems Jun 28, 2023

View reviewed changes

src/core/ddsc/src/dds_publisher.c Fixed Show fixed Hide fixed

src/core/ddsc/src/dds_reader.c Fixed Show fixed Hide fixed

MISRA rule 8-3 fix for more code in ddsc

da78bcf

eboasson reviewed Jun 30, 2023

View reviewed changes

Apply suggestion for rule 8-3

4deeb84

eboasson approved these changes Jul 3, 2023

View reviewed changes

eboasson merged commit dd43d2f into eclipse-cyclonedds:master Jul 3, 2023

PatrickM-ZS mentioned this pull request Jul 17, 2023

Update Zephyr port to support Zephyr V3.4 #1770

Merged

Splinter1984 deleted the fix-misra-rule-8-3 branch April 29, 2024 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MISRA rule 8-3(1/2) 'declarations of a function same name and type' #1742

Fix MISRA rule 8-3(1/2) 'declarations of a function same name and type' #1742

Splinter1984 commented Jun 28, 2023

eboasson left a comment

eboasson Jun 30, 2023

eboasson Jun 30, 2023

Splinter1984 Jun 30, 2023

eboasson Jun 30, 2023

Splinter1984 Jun 30, 2023

eboasson Jun 30, 2023

Splinter1984 Jun 30, 2023

eboasson Jun 30, 2023

eboasson Jun 30, 2023

Splinter1984 Jun 30, 2023

eboasson Jun 30, 2023

Splinter1984 Jun 30, 2023

eboasson Jun 30, 2023

Splinter1984 Jun 30, 2023

eboasson Jun 30, 2023

Splinter1984 Jun 30, 2023

eboasson Jun 30, 2023

Splinter1984 Jun 30, 2023

eboasson Jun 30, 2023

Splinter1984 Jun 30, 2023

eboasson left a comment

	#define DDS_GET_STATUS_COMMON(ent_type_, status_) \
	dds_return_t dds_get_##status_##_status (dds_entity_t entity, dds_##status_##_status_t *status) \
	{ \
	dds_##ent_type_ *ent; \
	dds_return_t ret; \
	if ((ret = dds_##ent_type_##_lock (entity, &ent)) != DDS_RETCODE_OK) \
	return ret; \
	ddsrt_mutex_lock (&ent->m_entity.m_observers_lock); \
	dds_get_##status_##_status_locked (ent, status); \
	ddsrt_mutex_unlock (&ent->m_entity.m_observers_lock); \
	dds_##ent_type_##_unlock (ent); \
	return DDS_RETCODE_OK; \
	}

Fix MISRA rule 8-3(1/2) 'declarations of a function same name and type' #1742

Fix MISRA rule 8-3(1/2) 'declarations of a function same name and type' #1742

Conversation

Splinter1984 commented Jun 28, 2023

eboasson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eboasson left a comment

Choose a reason for hiding this comment