negative values cannot be stored to size_t #53
Conversation
|
recreated my branch to cleaning up my commits. (perhaps there might have been better ways to do the same, but as a beginner I could not come up with the idea. |
| slotref slot = slotat(slot_ref); | ||
| if (slot) | ||
| { | ||
| index = seg.findClassIndex(input_class, slot->gid()); | ||
| is->setGlyph(&seg, seg.getClassGlyph(output_class, index)); | ||
| asset(index >= 0); | ||
| is->setGlyph(&seg, seg.getClassGlyph(output_class, static_cast<uint16>(index))); |
There was a problem hiding this comment.
The assert can be helpful, but only has effect in debug builds. Otherwise this forces a call to getClassGlyph with the index 31767 in this case. If the idea in the first place is to avoid calling setGlyph with an invalid glyph, then guarding the call with an "if (index > 0)" might be appropiate?
There was a problem hiding this comment.
true, "if(index >= 0)" thingy would be more helpful, but I wasn't sure what I should do in error handling. Just returning without doing anything? showing a dialog that alerts user about the error? throwing an exception?
There was a problem hiding this comment.
Please stop slamming asserts in everywhere where you have just switched from size_t to int because you know it may well return something < 0! If you static cast from int to uint16 what's so bad about having the cast occur earlier?
Graphite does not throw exceptions. That is a design constraint to allow it to have a pure C interface and not be dependent on libstdc++.
| void *pGlyph = TtfUtil::GlyfLookup(_glyf, locidx, _glyf.size()); | ||
| int locidx = TtfUtil::LocaLookup(glyphid, _loca, _loca.size(), _head); | ||
| assert(locidx >= 0); | ||
| void *pGlyph = TtfUtil::GlyfLookup(_glyf, static_cast<size_t>(locidx), _glyf.size()); |
There was a problem hiding this comment.
Depending on the necessity of having a value filled into that bbox further down in this method, I suggest something like
Glyph pGlyph = nullptr; / is there such a generic type to avoid all the casting? */
int locidx = ...LocaLookup...;
if (locidx >= 0) pGlyph = ...GlyfLookup...;
if (pGlyph && ...GlyfBox...) ...etc...
There was a problem hiding this comment.
LocaLookup returns -1 and -2 as sentinel error values which are tested for in GlyfLookup. The assert() would cause a crash on faulty fonts rather than handling things tidily. Since -1 and -2 are sentinels it was felt adequate that the type is size_t (given that -1 and -2 as size_t are just big positive numbers adequately represented by -1 and -2). This modification doesn't add anything. And the assert introduces a bug.
| @@ -601,7 +605,8 @@ STARTOP(put_subs) | |||
| if (slot) | |||
| { | |||
| int index = seg.findClassIndex(input_class, slot->gid()); | |||
| is->setGlyph(&seg, seg.getClassGlyph(output_class, index)); | |||
| asset(index >=0 ); | |||
| is->setGlyph(&seg, seg.getClassGlyph(output_class, static_cast<uint16>(index))); | |||
There was a problem hiding this comment.
The question I would ask here for the sake of robustness: What does actually getClassGlyph return when called with an index like 31767 (from -1), and how does setGlyph react to getting fed that return value?
There was a problem hiding this comment.
My initial intention of the code was limited to reducing warnings on locally building LibreOffice,which relies on this component, and haven't researched what would happen in that case, but making use of the opportunity of being asked, I tried to understand terms used in the code, like font,face,segment, slif , GDL and more. However, the links to useful information on README of this project were broken. Sorry, I'm currently at a loss.
For now, I found that I typo-ed "assert" as "asset". I will fix that.
There was a problem hiding this comment.
If getClassGlyph is called with a an index that is > than the limit it can handle, it returns 0 (and since the index is uint, it is never < 0). This gives us an adequate garbage in, garbage out error handling. The Graphite engine is not in an a position to return errors, it has to do something with whatever is thrown at it, even if the results are rubbish.
There was a problem hiding this comment.
Is perhaps the value or index 0 itself also classified as such a special reference value, or an error condition? The last comment seems to suggest so. E.g. the index 0 could indeed have been utilized as a valid but special index to hold, say, the "substitute glyph" to use as fallback or during "silent recovery" whenever the libray is thrown some bad input.
…rns -1, contradicting its declaration
himajin100000
force-pushed
the
size_t_to_int
branch
from
711bd03
to
cdbccaf
Compare
| @@ -309,7 +309,7 @@ uint16 Silf::findPseudo(uint32 uid) const | |||
| return 0; | |||
| } | |||
|
|
|||
| uint16 Silf::findClassIndex(uint16 cid, uint16 gid) const | |||
| int Silf::findClassIndex(uint16 cid, uint16 gid) const | |||
There was a problem hiding this comment.
The same principle of sentinels applies here.
No description provided.