Skip to content

issue #241: add infrastructure for range checking#296

Open
seanm wants to merge 2 commits into
tbeu:masterfrom
seanm:issue241-try2
Open

issue #241: add infrastructure for range checking#296
seanm wants to merge 2 commits into
tbeu:masterfrom
seanm:issue241-try2

Conversation

@seanm

@seanm seanm commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@seanm

seanm commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

@tbeu I still need to fill out all the variants of the conversion functions, but any thoughts on this general approach?

@seanm

seanm commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

@tbeu I still need to fill out all the variants of the conversion functions, but any thoughts on this general approach?

@tbeu friendly ping

@tbeu tbeu force-pushed the issue241-try2 branch from e883884 to 5df3925 Compare May 16, 2026 18:01
@tbeu tbeu linked an issue May 18, 2026 that may be closed by this pull request
READ_DATA macro needs checks before float to int casts #241
Open
@tbeu

tbeu commented May 18, 2026

Copy link
Copy Markdown
Owner

CI is failing. This needs to be addressed.

@seanm

seanm commented May 19, 2026

Copy link
Copy Markdown
Contributor Author

CI is failing. This needs to be addressed.

That's expected. This is WIP. I'd like to know if you find the general approach acceptable before I spend more time to finish it off.

@tbeu

tbeu commented May 20, 2026

Copy link
Copy Markdown
Owner

So well, if it works and solves the issue...

@seanm

seanm commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

So well, if it works and solves the issue...

OK I'll finish it off then...

@seanm seanm force-pushed the issue241-try2 branch 2 times, most recently from 8726eab to 2c6094d Compare May 20, 2026 15:57
@seanm seanm changed the title WIP: issue #241: range checking issue #241: add infrastructure for range checking May 20, 2026
@seanm

seanm commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

@tbeu ok I've decided to split this into 2 PRs since there's 1 test failing that needs investigation.

So for now, this first PR only adds the the infrastructure to eventually fix the UB conversions. It should change no behaviour. It's ready for review and merging.

tbeu
tbeu requested changes Jun 22, 2026
View reviewed changes

@tbeu tbeu left a comment •
edited
Loading

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Appveyor fails.

@seanm

seanm commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Appveyor fails.

Hmmm,

  ..\..\src\read_data.c(421): error C2054: expected '(' to follow 'inline' [C:\projects\matio\visual_studio\libmatio\libmatio.vcxproj]
  ..\..\src\read_data.c(422): error C2085: 'ConvertFromUInt16ToUInt16' : not in formal parameter list [C:\projects\matio\visual_studio\libmatio\libmatio.vcxproj]
  ..\..\src\read_data.c(422): error C2143: syntax error : missing ';' before '{' [C:\projects\matio\visual_studio\libmatio\libmatio.vcxproj]

Is this an old compiler? Or one that doesn't support C very well? Maybe it does not like static and inline together?

@seanm

seanm commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Some searching suggests indeed MS compilers don't support C well, and might not like static inline. I've just removed the inline keyword, we'll see if that fixes it... Such a tiny function will probably be inlined anyway.

tbeu
tbeu reviewed Jun 22, 2026
View reviewed changes
Comment thread src/read_data_impl.h Outdated
#define READ_TYPE_UINT64_DATA CAT(READ_TYPED_FUNC1, UInt64)
#endif /* HAVE_MAT_UINT64_T */

// Converts value from its own type to READ_TYPE, carefully ensuring the value fits, otherwise sets success to false.

@tbeu tbeu Jun 22, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
// Converts value from its own type to READ_TYPE, carefully ensuring the value fits, otherwise sets success to false.
// Stubs only: Converts value from its own type to READ_TYPE.

@seanm seanm Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, done. Indeed that comment is not yet true, but it will be in the next PR.

@seanm
Added infrastructure for non-UB conversion between various types
ee3fd00 
Modified READ_DATA macro to:

- know both source and destination type
- provide a function to convert between types

For now the conversion function just casts, which is what the macro did before. In other words, this commit should not change any behaviour at all. It only adds infrastructure.

Subsequently we can carefully implement the conversion functions to not invoke undefined behaviour when converting, for example, large ints to small ints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbeu tbeu tbeu requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

READ_DATA macro needs checks before float to int casts

2 participants

@seanm @tbeu