Skip to content

[compute] Explicitly initialize dims_ in constructors to avoid uninitialized warnings#15155

Merged
chunseoklee merged 1 commit intoSamsung:masterfrom
ragmani:cker/fix_maybe_uninitialized
Apr 14, 2025
Merged

[compute] Explicitly initialize dims_ in constructors to avoid uninitialized warnings#15155
chunseoklee merged 1 commit intoSamsung:masterfrom
ragmani:cker/fix_maybe_uninitialized

Conversation

@ragmani
Copy link
Copy Markdown
Contributor

@ragmani ragmani commented Apr 11, 2025

This commit explicitly initialize dims_ in Shape constructors to avoid uninitialized warnings.

  • Set dims_ using std::array or std::vector in the (int, int32_t), (int, const int32_t*), and initializer-list constructors to ensure proper initialization before calling Resize().
  • Added a check in Resize() using dims_.valueless_by_exception() to handle any uninitialized state.

ONE-DCO-1.0-Signed-off-by: ragmani ragmani0216@gmail.com

…ialized warnings

This commit explicitly initialize dims_ in Shape constructors to avoid uninitialized warnings.
- Set dims_ using std::array or std::vector in the (int, int32_t), (int, const int32_t*),
  and initializer-list constructors to ensure proper initialization before calling Resize().
- Added a check in Resize() using dims_.valueless_by_exception() to handle any uninitialized state.

ONE-DCO-1.0-Signed-off-by: ragmani <ragmani0216@gmail.com>
@ragmani
Copy link
Copy Markdown
Contributor Author

ragmani commented Apr 11, 2025

To fix the build error(#15030 (comment)), This PR addresses a problem where the Resize() function could access an uninitialized member (dims_). Unfortunately, modifying Resize() alone did not prevent the "maybe-uninitialized" warnings and I couldn't find other alternative approaches to avoid it, Therefore I updated some constructors to explicitly initialize dims_ using the correct alternative (std::array or std::vector) based on the dimensions count. Regrettably, these changes have made the Resize() function considerably longer than before.

@ragmani ragmani requested review from a team and glistening April 11, 2025 12:19
@ragmani ragmani added PR/ready for review It is ready to review. Please review it. Urgent It's an urgent task labels Apr 11, 2025
Copy link
Copy Markdown
Contributor

@seockho-kim seockho-kim left a comment

Choose a reason for hiding this comment

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

LGTM

@ragmani ragmani removed the request for review from glistening April 14, 2025 02:07
@chunseoklee chunseoklee merged commit 8253eb5 into Samsung:master Apr 14, 2025
10 checks passed
@glistening
Copy link
Copy Markdown
Contributor

glistening commented Apr 14, 2025

I was reading the code before merging. Personally I don't like to merge this.

  • It introduces duplicated code segments. I am not sure it is necessary.
  • The code becomes more complicated and diverged from original code base, which was copied from tensorflow lite. At least, I don't understand the changed code.

As I wrote a few hours ago in #15157, I am investigating the reason, rather than rewriting and modifying the existing code to c++17. I expect the required change would be just a few lines or a few bytes.

@ragmani
Copy link
Copy Markdown
Contributor Author

ragmani commented Apr 14, 2025

My priority is to ensure that the build doesn’t break, so for now, I'll implement fixes to avoid any build errors. Since I am only modifying the Shape class, if you're not happy with the changes, you can easily revert the modifications in the Shape file by replacing the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR/ready for review It is ready to review. Please review it. Urgent It's an urgent task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants