Modernize FX_BIDICLASS, FX_BREAKPROPERTY, and FX_CHARTYPE names [pdfium : main]
Nicolás Peña (Gerrit)
Nicolás Peña added 4 comments![]( "Open in Gerrit")
return static_cast<BidiClass>((val >> 4) & 0x0F);What's this magic number? Make it a constant with a clear name? Used elsewhere too, sometimes as 0xF
if (eClsRun != static_cast<BidiClass>(0xF) && iNum > 0) {Perhaps this is the same constant?
CharType chartype = pdfium::unicode::GetCharType(wch);char_type for consistency with char_type_?
Related details
- Helmut Januschka
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
return static_cast<BidiClass>((val >> 4) & 0x0F);What's this magic number? Make it a constant with a clear name? Used elsewhere too, sometimes as 0xF
Done. Added named constants `kNibbleBits`/`kNibbleMask` for the nibble shift/mask, and `kBidiClassNone` for the `0xF` "no BidiClass" sentinel. Replaced the magic numbers
if (eClsRun != static_cast<BidiClass>(0xF) && iNum > 0) {Perhaps this is the same constant?
Acknowledged
CharType chartype = pdfium::unicode::GetCharType(wch);char_type for consistency with char_type_?
- Lei Zhang
- Nicolás Peña
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
Only concern is a loss of uniqueness in these names. It seems unlikely that any of these symbols would persist outside of compile phase, but one thing we've done in the past is in the header do ...
```
namespace pdfium {
enum class BidiClass { ... };
} // namespace pdfium
using pdfium::BidiClass;
```
just to avoid some latent linker hassle.
Related details
- Helmut Januschka
- Lei Zhang
- Nicolás Peña
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolás Peña (Gerrit)
Nicolás Peña added 1 comment![]( "Open in Gerrit")
constexpr BidiClass kBidiClassNone = static_cast<BidiClass>(kNibbleMask);This definition doesn't seem quite right, or is at least confusing, because 0x0F is actually a valid BidiClass value (kRLE).
Related details
- Helmut Januschka
- Lei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
constexpr BidiClass kBidiClassNone = static_cast<BidiClass>(kNibbleMask);This definition doesn't seem quite right, or is at least confusing, because 0x0F is actually a valid BidiClass value (kRLE).
- Lei Zhang
- Nicolás Peña
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolás Peña (Gerrit)
Nicolás Peña added 1 comment![]( "Open in Gerrit")
constexpr BidiClass kBidiClassNone = static_cast<BidiClass>(kNibbleMask);Helmut JanuschkaThis definition doesn't seem quite right, or is at least confusing, because 0x0F is actually a valid BidiClass value (kRLE).
oh, messed it up
- Helmut Januschka
- Lei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
constexpr BidiClass kBidiClassNone = static_cast<BidiClass>(kNibbleMask);Helmut JanuschkaThis definition doesn't seem quite right, or is at least confusing, because 0x0F is actually a valid BidiClass value (kRLE).
Nicolás Peñaoh, messed it up
Did you mean to upload a new patchset?
yes, should be correct now!
Dropped the constant and inlined `0x0F` at the use sites. Kept the `kNibbleBits`/`kNibbleMask` cleanup.
Related details
- Lei Zhang
- Nicolás Peña
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolás Peña (Gerrit)
Nicolás Peña voted Code-Review+1![]( "Open in Gerrit")
- Helmut Januschka
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
Only concern is a loss of uniqueness in these names. It seems unlikely that any of these symbols would persist outside of compile phase, but one thing we've done in the past is in the header do ...
```
namespace pdfium {
enum class BidiClass { ... };
} // namespace pdfiumusing pdfium::BidiClass;
```
just to avoid some latent linker hassle.
Please try extending the namespace in fx_unicode.h to include these enums.
Bug: 42270078This CL doesn't remove any #defines though.
Related details
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |