Skip to content

[ggma] Refactor GGMA API implementation#16260

Merged
hseok-oh merged 2 commits intoSamsung:masterfrom
glistening:ggma_camel
Nov 3, 2025
Merged

[ggma] Refactor GGMA API implementation#16260
hseok-oh merged 2 commits intoSamsung:masterfrom
glistening:ggma_camel

Conversation

@glistening
Copy link
Copy Markdown
Contributor

Refactoring is done while introducing tokenize module..

  • Use Camel-style file name for internal files It is the same way of onert. (e.g. context.h -> Context.h)
  • Move common macro to ggma_macro
  • All ggma-api-header dependency is in ggma_*.cc
  • Internal files doesn't know ggma_types any longer.

ONE-DCO-1.0-Signed-off-by: Sanggyu Lee sg5.lee@samsung.com

@glistening glistening requested review from a team, chunseoklee and hseok-oh October 31, 2025 02:04
@glistening glistening changed the title [onert/ggma] Refactor GGMA API implementation [ggma] Refactor GGMA API implementation Oct 31, 2025
@glistening glistening added the PR/ready for review It is ready to review. Please review it. label Oct 31, 2025
hseok-oh
hseok-oh previously approved these changes Nov 3, 2025
Copy link
Copy Markdown
Contributor

@hseok-oh hseok-oh left a comment

Choose a reason for hiding this comment

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

LGTM

@hseok-oh
Copy link
Copy Markdown
Contributor

hseok-oh commented Nov 3, 2025

@glistening Not related with this PR, but ggma_macro.h can be moved into src/, not include/ IMHO. Is there any reason to place include/ and install as public header?

@glistening
Copy link
Copy Markdown
Contributor Author

glistening commented Nov 3, 2025

@hseok-oh

@glistening Not related with this PR, but ggma_macro.h can be moved into src/, not include/ IMHO. Is there any reason to place include/ and install as public header?

We have duplicated nnfw_util.h (which contains NNPR_ENSURE_STATUS) under

  • runtime/tests/tools/onert_run/src/nnfw_util.h
  • runtime/tests/tools/onert_train/src/nnfw_util.h

It will get another duplications for every new tools or something uses nnfw APIs.

I think it is benefitial to provide GGMA_ENSURE as a part of public interface to remove duplication. It would reduce our maintenance effort, also good for SE score.

If you think it is a burdeb, we may move include/ggma_macro.h under src/Macro.h.
(Or move GGMA_UNUSED and GGMA_RETURN_ERROR_IF_NULL to src/Macro.h since they are for API implementer, not API user.)

@hseok-oh
Copy link
Copy Markdown
Contributor

hseok-oh commented Nov 3, 2025

It would reduce our maintenance effort, also good for SE score.

Because they are test tools, they are not effects to SE score.

If we want to reduce duplication of nnfw_util.h, we can move them into tests/tools/libs. I have a question that is it meaningful to publish ggma_macro.h as public header as a part of API in developer package.

@glistening
Copy link
Copy Markdown
Contributor Author

glistening commented Nov 3, 2025

It would reduce our maintenance effort, also good for SE score.

Because they are test tools, they are not effects to SE score.

Yes, we can ignore the score for test tools.

But test tool is just an example of our API use case.

The duplication - specifically the tedious and repetitive error checking - occurs across all API use cases.
This is why including GGMA_ENSURE would be beneficial.

If we want to reduce duplication of nnfw_util.h, we can move them into tests/tools/libs. I have a question that is it meaningful to publish ggma_macro.h as public header as a part of API in developer package.

It only helps the internal tools developers.

However, it is likely there is not much users at this moment.
We may not publish ggma_macro.h at this moment.
Both (publish or not publish) work for me.

Refactoring is done while introducing tokenize module..

- Use Camel-style file name for internal files
  It is the same way of onert. (e.g. context.h -> Context.h)
- Move common macro to ggma_macro
- All ggma-api-header dependency is in ggma_*.cc
- Internal files doesn't know ggma_types any longer.

ONE-DCO-1.0-Signed-off-by: Sanggyu Lee <sg5.lee@samsung.com>
Comment thread runtime/ggma/src/Macro.h
{ \
exit(-1); \
} \
} while (0)
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.

It will be under ggma_run.cc.
Then, it will be in tests/tools/libs for two or more tools.
Then, it will be in public header if other use cases occurrs.

Copy link
Copy Markdown
Contributor

@hseok-oh hseok-oh left a comment

Choose a reason for hiding this comment

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

LGTM

@hseok-oh hseok-oh merged commit 2e8f43a into Samsung:master Nov 3, 2025
10 checks passed
@glistening glistening deleted the ggma_camel branch November 4, 2025 01:17
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants