You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
The algorithm in the standard is the yes_only = false (or want_definite = true) case, so I think things are simplest to describe if that's the baseline and the other case is described by contrast with it. Could certainly handle that with some added words, though.
Another aspect is that there is an asymmetry between a definite YES and a definite NO. In the yes_only / !want_definite case, what the caller really most wants is a definite YES... it's just a definite NO that is no more helpful than a MAYBE. Or another angle on that: if the caller really didn't care about getting a definite answer, it could just assume MAYBE and not bother calling the helper 😉 It's in the hope of getting a YES that it makes the call.
More ideas just now: perhaps maybe_is_no, to mean "a MAYBE is no worse than a NO"?
Or yes_or_maybe, for "there are only two distinct answers, YES and MAYBE". That's basically what I was going for with yes_only, but probably a less paradoxical way of putting it 🙂
The reason will be displayed to describe this comment to others. Learn more.
The API also surprised me. I suggest tho rename is_normalized_quickcheck() to is_normalized_impl() and add 2 functions: is_normalized() (yes_only=false) and is_normalized_quickcheck() (yes_only=true).
The reason will be displayed to describe this comment to others. Learn more.
I suggest to rename is_normalized_quickcheck() to is_normalized_impl() and add 2 functions: is_normalized() (yes_only=false) and is_normalized_quickcheck() (yes_only=true).
Hmm, I like the idea of covering over some of the multiplication of flags with wrapper functions!
I don't like quite these names, though. First, I don't think is_normalized is a good name, for either value of the flag, because the function doesn't actually take full responsibility for finding out if the string is normalized -- it can return MAYBE, and then it's up to the caller to go and normalize the string and compare.
What the function does do is implement a quick but partial check to try to see if the string is normalized. Specifically it implements one that's defined in the standard, which the standard calls quickCheck in its sample code, and which uses a property in the UCD called Quick_Check.
More precisely, the yes_only=false case implements that standard quick-check algorithm; the yes_only=true (or yes_or_maybe=true, etc.) case (which is the version in master) is a variation on it. I think the name "quickcheck" or is_normalized_quickcheck, with no further qualifications, sounds like it will mean the standard's algorithm and it's confusing if it means a different algorithm instead.
Here's another possible set of names to use with the same idea:
rename this function to is_normalized_quickcheck_impl
is_normalized_quickcheck is the standard's quickcheck (yes_only/yes_or_maybe=false)
is_normalized_quickercheck is the variant that's more impatient to return and hand responsibility back to the caller (yes_or_maybe=true)
Or is_normalized_quickcheck_quicker
Or if those names feel too long: is_normalized_qc_impl, is_normalized_qc, is_normalized_qc_quicker.
The reason will be displayed to describe this comment to others. Learn more.
Oh wow it's the future! 🙂 I guess that's a C99 feature, and one of those we now accept using. Good to know.
I'm not sure I want to convert the existing two parameters within this PR; it seems like that could muddy the diff a bit with changes that are orthogonal to what it's really doing. But I guess I can see how it looks.
The reason will be displayed to describe this comment to others. Learn more.
Pushed a change to bool for the new parameter.
I was a bit surprised to find that I needed to add an #include <stdbool.h> line to the file! Perhaps that should be in a central include file, in the same way as limits.h and inttypes.h are. That's certainly orthogonal to this PR, though.
The reason will be displayed to describe this comment to others. Learn more.
I made a quick further commit converting nfc and k to bool too (including in the callers, and using true/false rather than 1/0 for literal values.)
I'm ambivalent about adding it here. It expands the diff a bit -- was:
1 file changed, 51 insertions(+), 24 deletions(-)
and becomes:
1 file changed, 57 insertions(+), 30 deletions(-)
Slightly inclined to leave it out, but if you'd rather include it I'll do that.
The reason will be displayed to describe this comment to others. Learn more.
I think we want to avoid polluting public headers with a stdbool.h include—bool is a bit of a name grab in arbitrary mature C codebases. Using stdbool.h where it's needed is also consistent with IWYU.
The reason will be displayed to describe this comment to others. Learn more.
I think we want to avoid polluting public headers with a stdbool.h include—bool is a bit of a name grab in arbitrary mature C codebases.
Ah, fair enough. I think that's consistent with putting it in a generic internal-only header, though. Within the tree, the very few existing uses are either stdbool, or (in Modules/expat/xmltok.c) a sort of manual stdbool.
Using stdbool.h where it's needed is also consistent with IWYU.
Hmm true. I guess IWYU doesn't feel like how this codebase is mostly written... but maybe it's best to make it more so.
The reason will be displayed to describe this comment to others. Learn more.
What makes the diffstat larger? Aren't you already touching all callers, since you added a parameter?
Here's the extra commit: gnprice@896c970f5
The lines not otherwise touched are where we declare and then fiddle with the nfc and k locals in unicodedata_UCD_is_normalized_impl.
/* An older version of the database is requested, quickchecks must be
disabled. */
if (self && UCD_Check(self))
Expand Down
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes_onlyis a bizarre name for a parameter that causes the function to returnMAYBE.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not really a fan of this name; I'd be glad for a better one.
The idea is that this flag says the caller doesn't care about NO vs. MAYBE; it only cares whether the answer is YES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe invert the sense and call it
want_definitemeaning "process the string until we're sure it's not normalized or we reach the end"?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithm in the standard is the
yes_only = false(orwant_definite = true) case, so I think things are simplest to describe if that's the baseline and the other case is described by contrast with it. Could certainly handle that with some added words, though.Another aspect is that there is an asymmetry between a definite YES and a definite NO. In the
yes_only/!want_definitecase, what the caller really most wants is a definite YES... it's just a definite NO that is no more helpful than a MAYBE. Or another angle on that: if the caller really didn't care about getting a definite answer, it could just assume MAYBE and not bother calling the helper 😉 It's in the hope of getting a YES that it makes the call.More ideas just now: perhaps
maybe_is_no, to mean "a MAYBE is no worse than a NO"?Or
yes_or_maybe, for "there are only two distinct answers, YES and MAYBE". That's basically what I was going for withyes_only, but probably a less paradoxical way of putting it 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API also surprised me. I suggest tho rename is_normalized_quickcheck() to is_normalized_impl() and add 2 functions: is_normalized() (yes_only=false) and is_normalized_quickcheck() (yes_only=true).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I like the idea of covering over some of the multiplication of flags with wrapper functions!
I don't like quite these names, though. First, I don't think
is_normalizedis a good name, for either value of the flag, because the function doesn't actually take full responsibility for finding out if the string is normalized -- it can return MAYBE, and then it's up to the caller to go and normalize the string and compare.What the function does do is implement a quick but partial check to try to see if the string is normalized. Specifically it implements one that's defined in the standard, which the standard calls
quickCheckin its sample code, and which uses a property in the UCD calledQuick_Check.More precisely, the yes_only=false case implements that standard quick-check algorithm; the yes_only=true (or yes_or_maybe=true, etc.) case (which is the version in master) is a variation on it. I think the name "quickcheck" or
is_normalized_quickcheck, with no further qualifications, sounds like it will mean the standard's algorithm and it's confusing if it means a different algorithm instead.Here's another possible set of names to use with the same idea:
is_normalized_quickcheck_implis_normalized_quickcheckis the standard's quickcheck (yes_only/yes_or_maybe=false)is_normalized_quickercheckis the variant that's more impatient toreturnand hand responsibility back to the caller (yes_or_maybe=true)is_normalized_quickcheck_quickerOr if those names feel too long:
is_normalized_qc_impl,is_normalized_qc,is_normalized_qc_quicker.How does that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea for the flag name (though its name becomes less important if we use Victor's idea of a pair of wrapper functions):
yes_vs_maybe.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like anything is standing out as dramatically better. At least the semantics are clearly documented and easily read from the function.
So, let's go with the current state.