malformed utf8 message: small cleanups

[happy-barney]

• use symbol names instead of magic number with comment
• transpose die/warn argument into symbol (macro name)
• make flags argument optional
• encapsulate lookup-and-die combo into single symbol (macro)

[iabyn]

I'm really not keen on the second commit. Most of the macros don't really add value, so instead they just make things more obscure - someone reading the code has to look up the source of the macro to understand what it's doing. For the one macro ENSURE_NOT_MALFORMED_UTF8 which actually does something substantial, it's only used once, so it seems to me that a simple code comment along the lines of '/* ensure the the utf8 is not malformed */' would be more useful. Unless you envisage this macro subsequently used in multiple places?
But in any case, things like this should be done as static functions these days rather than macros.

[happy-barney]

For the one macro ENSURE_NOT_MALFORMED_UTF8 which actually does something substantial, it's only used once

It is actually used 4 times.

I've created this macros to pinpoint calling differences for now. In my experience when programmers see such small differences they usually come up with subsequent cleanups leading to less code - but you have to formalize intention first.

[iabyn]

On Mon, Nov 06, 2023 at 03:18:14AM -0800, Branislav Zahradník wrote: > For the one macro ENSURE_NOT_MALFORMED_UTF8 which actually does something substantial, it's only used once It is actually used 4 times.

ah sorry, I misread the code.

I've created this macros to pinpoint calling differences for now. In my experience when programmers see such small differences they usually come up with subsequent cleanups leading to less code - but you have to formalize intention first.

So I don't object now to ENSURE_NOT_MALFORMED_UTF8() being added, but as an inline function rather than than a macro. I still don't like the other proposed macros. As a long-running code-base, we've learned to become cautious about code churn. More often that not, attempts to "tidy up the code" etc just risk introducing new bugs for relatively little potential gain.

…

-- My get-up-and-go just got up and went.

[happy-barney]

So I don't object now to ENSURE_NOT_MALFORMED_UTF8() being added, but as an inline function rather than than a macro.

located where ?

I still don't like the other proposed macros.

inline functions?

As a long-running code-base, we've learned to become cautious about code churn. More often that not, attempts to "tidy up the code" etc just risk introducing new bugs for relatively little potential gain.

depends on approach, small changes like this vs few hundreds commits modifying thousands of lines ...

[iabyn]

On Tue, Nov 07, 2023 at 04:25:48AM -0800, Branislav Zahradník wrote: > So I don't object now to ENSURE_NOT_MALFORMED_UTF8() being added, but as an inline function rather than than a macro. located where ?

inline.h usually.

> I still don't like the other proposed macros. inline functions?

However they're implemented, I don't like them.

…

-- "I used to be with it, but then they changed what ‘it’ was, and now what I’m with isn’t it. And what’s ‘it’ seems weird and scary to me." -- Grandpa Simpson (It will happen to you too.)

[happy-barney]

On Tue, Nov 07, 2023 at 04:25:48AM -0800, Branislav Zahradník wrote: > So I don't object now to ENSURE_NOT_MALFORMED_UTF8() being added, but as an inline function rather than than a macro. located where ?
inline.h usually.

there is one issue ... macro generates NOT_REACHED, currently it is used in caller

[iabyn]

On Thu, Nov 09, 2023 at 07:08:41AM -0800, Branislav Zahradník wrote: > On Tue, Nov 07, 2023 at 04:25:48AM -0800, Branislav Zahradník wrote: > So I don't object now to ENSURE_NOT_MALFORMED_UTF8() being added, but as an inline function rather than than a macro. located where ? > inline.h usually. there is one issue ... macro generates `NOT_REACHED`, currently it is used in caller

I don't understand what issue this causes for you. The NOT_REACHED is within a conditional in the macro / inline function. So just include include it in the inline function definition, e.g. PERL_STATIC_INLINE Perl_ensure_not_malformed_utf8(...) { ... if (UNLIKELY(has_malformed_char)) { ....; NOT_REACHED; } } Thinking a bit further, does the function need to be inline? Is it performance critical? If not, a normal function would be more appropriate. One final thought: is this new function intended to be for core use only or fas part of the general API? If the latter, then more consideration would be needed in its API design.

…

-- The Enterprise is captured by a vastly superior alien intelligence which does not put them on trial. -- Things That Never Happen in "Star Trek" #10

[jkeenan]

@happy-barney, before we go further with this pull request, could you investigate the test failure being reported by our CI here? Please run make test_porting and get a PASS before re-pushing your branch. Thanks.

[happy-barney]

@jkeenan I pushed because I had problem to run tests at all (complains about missing Math-BigRat)

Now I'm looking into what is causing this - somewhere this check is called on zero-length string (quickfix - skip generated assert(length))

[jkeenan]

@jkeenan I pushed because I had problem to run tests at all (complains about missing Math-BigRat)

Now I'm looking into what is causing this - somewhere this check is called on zero-length string (quickfix - skip generated assert(length))

There has been no discussion in this ticket in approximately 9 months. It has acquired merge conflicts in the interim. The code reviewer was skeptical of several parts of the code changes. I doubt we're going to move forward with this, so I'm labeling it 'Closable?' for now.

[happy-barney]

I can fix conflict, but question is, will it be merged then?

[khwilliamson]

@happy-barney I would merge the first commit, and work with you on the second

[happy-barney]

@khwilliamson ok, so rebasing with first commit only

[khwilliamson]

It's still showing conflicts

[khwilliamson]

@happy-barney please create a new P.R. with the other commit, and we can work on it