[PSA Client Testing] Re-introduce c_parsing_helper.pr and c_wrapper_generator.py #9053
Conversation
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
minosgalanakis
added
needs-review
tom-cosgrove-arm
changed the title
There was a problem hiding this comment.
I've read the code provided by the 2 python files and it looks OK to me. As final check I removed tests/src/psa_test_wrappers.c and tests/include/test/psa_test_wrappers.h and re-generated them using ./tests/scripts/generate_psa_wrappers.py. Generated files were identical to the previous ones.
So this PR is OK for me :)
| self.header_guard = None #type: Optional[str] | ||
|
|
||
| def _write_prologue(self, out: typing_util.Writable, header: bool) -> None: | ||
| """Write the prologue of a C file. |
There was a problem hiding this comment.
I am not a big fan of how this multiline string formatting is being handled here. It works but make the code extremely hard to review/understand.
I would be indendently declaring my c_template strings in the top of the module, and format it in the method.
e.g
header = """\
/* Automatically generated by {} do not edit! */
/* Copyright The Mbed TLS Contributors
* SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
*/
"""
def _write_prologue(out, program_name):
out.write(header.format(program_name))
Or if we need to control indentation, or do it in-line during the method I would use the textutils module.
def _write_prologue_alt(out, program_name):
header_alt = """\
/* Automatically generated by {} do not edit! */
/* Copyright The Mbed TLS Contributors
* SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
*/
"""
out.write(dedent(header_alt .format(program_name)))
There was a problem hiding this comment.
I disagree with defining strings outside of the function. That makes the code hard to read because you have to scroll back and forth all the time.
the textutils module
We're trying to cut down on external packages. It's definitely not worth it for something so minor. Or did you mean the built-in textwrap.dedent? That doesn't do what we need: it removes all indentation, but a lot of the code snippets need to have some indentation throughout.
| self._write_epilogue(out, True) | ||
|
|
||
|
|
||
| class UnknownTypeForPrintf(Exception): |
There was a problem hiding this comment.
Ideally exception classes are declared first on the module
|
|
||
| def _write_prologue(self, out: typing_util.Writable, header: bool) -> None: | ||
| super()._write_prologue(out, header) | ||
| if not header: |
There was a problem hiding this comment.
I think this is a bit confusing as to why it is needed? The Base()._write_prologue already has an if header -> do something branching logic.
My guess is to add the appropriate config/headers so that library is printing out log messages.
We should add a comment to explain what happens.
There was a problem hiding this comment.
In the .c file, we include the declarations for the functions that are used in the generated code. In the .h file, there's nothing extra to include here. I'm not sure what to add: the intent looks straightforward to me.
| #endif /* defined(MBEDTLS_FS_IO) && defined(MBEDTLS_TEST_HOOKS) */ | ||
| """) | ||
|
|
||
| _PRINTF_SIMPLE_FORMAT = { |
There was a problem hiding this comment.
I would move this and _PRINTF_TYPE_CAST to the beginning of the class.
Description
This is a PR to review already merged files on the code-base. Changes introduced as a result of that review will be part of a separate PR against development.
This is a requirement of #8961