cppcheck icon indicating copy to clipboard operation
cppcheck copied to clipboard

pass a char buffer to simplecpp instead of a stream

Open firewave opened this issue 1 year ago • 3 comments

firewave avatar May 03 '24 15:05 firewave

This requires https://github.com/danmar/simplecpp/pull/347 to merged and sync'd down first.

firewave avatar May 03 '24 15:05 firewave

I will try to put the refactoring which does not require the simplecpp changes into a separate PR.

firewave avatar May 03 '24 16:05 firewave

The callgrind CI job shows a small reduction in Ir: 64,070,509,276 -> 63,862,736,388.

firewave avatar May 16 '24 14:05 firewave

@danmar This requires a simplecpp release.

firewave avatar Jul 08 '24 08:07 firewave

This exposed a false negative: https://trac.cppcheck.net/ticket/12711.

firewave avatar Jul 15 '24 06:07 firewave

@danmar This still requires a simplecpp release.

firewave avatar Jul 24 '24 22:07 firewave

The callgrind CI job shows a small reduction in Ir: 64,070,509,276 -> 63,862,736,388.

Please let's take that with a grain of salt. It's the instruction count not a time estimate.

danmar avatar Sep 29 '24 08:09 danmar

Please let's take that with a grain of salt. It's the instruction count not a time estimate.

some kind of small benchmark that measures actual time would be interesting.

danmar avatar Sep 29 '24 08:09 danmar

some kind of small benchmark that measures actual time would be interesting.

I will provide additional values. This will also help with value evaluation stuff which is not executed in that job. I just should it to highlight that it even has a measurable impact without that.

firewave avatar Sep 29 '24 13:09 firewave

I like the interface that we pass the istream to simplecpp. it makes the interface more strict. a const char* parameter can be a filename, code, configuration, etc.

That just moves the potential mistakes to a different layer. You could still pass unterminated data to the stream or improperly size your buffer.

If we were using newer standards we could provide std::string_view and std::span overloads in the interface and deprecate the raw pointer one. I will file a ticket about that later. But those are also prone to error though because of lifetime stuff. None of the possible options appears to be 100% safe if you don't know what you are doing.

firewave avatar Sep 29 '24 13:09 firewave

That just moves the potential mistakes to a different layer.

Sorry I feel we talk about different things. I talked about type safety and you about bounds safety.

So ok we can still have bounds safety problems with the std::istream.

But developers sometimes mix up the parameters and with a const char* there is nothing that prevents that for instance the filename is passed by mistake to that parameter.

danmar avatar Sep 29 '24 14:09 danmar

But developers sometimes mix up the parameters and with a const char* there is nothing that prevents that for instance the filename is passed by mistake to that parameter.

My idea would be to deprecate the raw pointers when you have the later standard wrappers. As I said I will file a ticket about it and try to address it in this dev cycle.

I also do not know how many people use the interface since we modified those several times and never get any reports about it so I assume those user might be non-existent.

firewave avatar Sep 29 '24 22:09 firewave

a const char* parameter can be a filename, code, configuration, etc.

You should check the documentation before you use it but point taken.

Maybe we could use more descriptive names like Buffer or FromFile suffixes. That would also make sense for the TokenList::createTokens() functions.

I would do the function names in this PR and do the safer interface function in a follow-up since that needs some looking into.

firewave avatar Sep 30 '24 08:09 firewave

Maybe we could use more descriptive names like Buffer or FromFile suffixes. That would also make sense for the TokenList::createTokens() functions.

I did that - please have a look.

firewave avatar Sep 30 '24 12:09 firewave

I might also still need to add convenience stream versions to the interface. And we would need tests for that - I am not so sure anymore about adding essentially dead code.

firewave avatar Sep 30 '24 14:09 firewave

I just don't like this. A istream parameter is better than 2 const uint8_t*, int size parameters imo. An istream is also more explicit than a string_view or a span.

danmar avatar Oct 03 '24 11:10 danmar

I just don't like this. A istream parameter is better than 2 const uint8_t*, int size parameters imo. An istream is also more explicit than a string_view or a span.

I will add the other safer versions in this PR as well and try to find a way to opt into the unsafe version so it can still be used internally.

firewave avatar Oct 03 '24 22:10 firewave