You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pdfbox.apache.org by "DvonHolten (via GitHub)" <gi...@apache.org> on 2024/02/08 13:49:29 UTC
[PR] some ToDo s to make things better [pdfbox]
DvonHolten opened a new pull request, #184:
URL: https://github.com/apache/pdfbox/pull/184
- add a ToDo proposal avoid recompiling regex (twice!) on every use
- add a ToDo proposal to save a lot (?) of memory
- remove some trailing blanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "THausherr (via GitHub)" <gi...@apache.org>.
THausherr commented on PR #184:
URL: https://github.com/apache/pdfbox/pull/184#issuecomment-1935913727
You don't have to bother with the 250000 files, this is our regression test corpus and we run these once on a dedicated server before doing a release. Just test the files from PDFBOX-5176 and the one from the related issue. I mentioned the 250000 files to show how unlikely this is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "xzel23 (via GitHub)" <gi...@apache.org>.
xzel23 commented on PR #184:
URL: https://github.com/apache/pdfbox/pull/184#issuecomment-1934602716
Nice find.
For the flag thing, how about just changing isValid to this?
```
public boolean isValid()
{
return value != Long.MAX_VALUE && value != Long.MIN_VALUE;
}
```
This would also make the constructor and where it is called a little bit shorter because there'd be no need to pass the `valid`parameter.
And the `STATIC` array could just be initialised in a static block, IMHO that wouldn't cost too much memory, and get(long) would be much simpler:
```
public static COSInteger get(long val)
{
if (LOW <= val && val <= HIGH)
{
return STATIC[(int) val - LOW];
}
return new COSInteger(val, true);
}
```
`getInvalid(boolean maxValue)` can also be removed and we use just
```
protected static final COSInteger OUT_OF_RANGE_MAX = getInvalid(Long.MAX_VALUE);
protected static final COSInteger OUT_OF_RANGE_MIN = getInvalid(Long.MIN_VALUE);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "xzel23 (via GitHub)" <gi...@apache.org>.
xzel23 commented on PR #184:
URL: https://github.com/apache/pdfbox/pull/184#issuecomment-1935778873
@THausherr What do you think about the suggested change? I just "bought" the PDF spec yesterday (you have to buy it, but the price is 0 USD), and I couldn't find anything about COInteger to require Long.MIN_VALUE and Long.MAX_VALUE to map to valid COSInteger values.
@DvonHolten if Tilman and/or some other maintainer agrees to the proposed change, do you want to make the flag related change yourself or should I create a PR for that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "DvonHolten (via GitHub)" <gi...@apache.org>.
DvonHolten closed pull request #184: some ToDo s to make things better
URL: https://github.com/apache/pdfbox/pull/184
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "DvonHolten (via GitHub)" <gi...@apache.org>.
DvonHolten commented on PR #184:
URL: https://github.com/apache/pdfbox/pull/184#issuecomment-1934189407
ok, regex coming.
the flag-thing should have some investigation on how many objects are created. is it 10 or 10000?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "THausherr (via GitHub)" <gi...@apache.org>.
THausherr commented on PR #184:
URL: https://github.com/apache/pdfbox/pull/184#issuecomment-1935798170
I'm not deep enough into the COSInteger code, it is related to PDFBOX-5176 and any change (except removing `getInvalid()` because of https://twitter.com/unclebobmartin/status/1114137614377005057 ) should be checked with the file from there and with the file from the earlier issue. PDFBOX-5176 was created after running a regression test on 250000 PDF files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "THausherr (via GitHub)" <gi...@apache.org>.
THausherr commented on code in PR #184:
URL: https://github.com/apache/pdfbox/pull/184#discussion_r1484556201
##########
fontbox/src/main/java/org/apache/fontbox/ttf/GlyphSubstitutionTable.java:
##########
@@ -75,6 +76,13 @@ public class GlyphSubstitutionTable extends TTFTable
private GsubData gsubData;
+ /**
+ * The regex represents 4 'word characters' [a-zA-Z_0-9],
+ * see {@link java.util.regex.ASCII#WORD}<br>
+ * Note: the ' '-character is not matched!
+ */
+ static final Pattern wordPattern = Pattern.compile( "\\w{4}" );
Review Comment:
yeah, can be done for the trunk (needs jdk11). I'll use the previous suggestion for 2.0 and 3.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "DvonHolten (via GitHub)" <gi...@apache.org>.
DvonHolten commented on PR #184:
URL: https://github.com/apache/pdfbox/pull/184#issuecomment-1935906983
i can do it - check, where the invalid things are coming from and where they are checked. if all the Long-values must be available, a subclass may do it.
and i have a closer look at those 250000 pdf -files in PDFBOX-5176. i assume, they are just created to check the parser - not to render them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "THausherr (via GitHub)" <gi...@apache.org>.
THausherr commented on PR #184:
URL: https://github.com/apache/pdfbox/pull/184#issuecomment-1934181176
Feel free to add a commit about a compiled constant regex. I don't have an opinion on the other topic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "THausherr (via GitHub)" <gi...@apache.org>.
THausherr commented on PR #184:
URL: https://github.com/apache/pdfbox/pull/184#issuecomment-1937005527
I've now committed the javadoc changes and the pattern change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "THausherr (via GitHub)" <gi...@apache.org>.
THausherr commented on code in PR #184:
URL: https://github.com/apache/pdfbox/pull/184#discussion_r1484556201
##########
fontbox/src/main/java/org/apache/fontbox/ttf/GlyphSubstitutionTable.java:
##########
@@ -75,6 +76,13 @@ public class GlyphSubstitutionTable extends TTFTable
private GsubData gsubData;
+ /**
+ * The regex represents 4 'word characters' [a-zA-Z_0-9],
+ * see {@link java.util.regex.ASCII#WORD}<br>
+ * Note: the ' '-character is not matched!
+ */
+ static final Pattern wordPattern = Pattern.compile( "\\w{4}" );
Review Comment:
yeah, can be done for 3.0 and trunk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] some ToDo s to make things better [pdfbox]
Posted by "xzel23 (via GitHub)" <gi...@apache.org>.
xzel23 commented on code in PR #184:
URL: https://github.com/apache/pdfbox/pull/184#discussion_r1484481434
##########
fontbox/src/main/java/org/apache/fontbox/ttf/GlyphSubstitutionTable.java:
##########
@@ -75,6 +76,13 @@ public class GlyphSubstitutionTable extends TTFTable
private GsubData gsubData;
+ /**
+ * The regex represents 4 'word characters' [a-zA-Z_0-9],
+ * see {@link java.util.regex.ASCII#WORD}<br>
+ * Note: the ' '-character is not matched!
+ */
+ static final Pattern wordPattern = Pattern.compile( "\\w{4}" );
Review Comment:
The name should be all uppercase for constants (static final). It should also be private.
##########
fontbox/src/main/java/org/apache/fontbox/ttf/GlyphSubstitutionTable.java:
##########
@@ -75,6 +76,13 @@ public class GlyphSubstitutionTable extends TTFTable
private GsubData gsubData;
+ /**
+ * The regex represents 4 'word characters' [a-zA-Z_0-9],
+ * see {@link java.util.regex.ASCII#WORD}<br>
+ * Note: the ' '-character is not matched!
+ */
+ static final Pattern wordPattern = Pattern.compile( "\\w{4}" );
Review Comment:
What do you think about making this a predicate?
```
private static final Predicate<String> IS_FOUR_LETTERS = Pattern.compile( "\\w{4}" ).asMatchPredicate();
```
Then the test would be `IS_FOUR_LETTERS.test(featureTags[i])`.
##########
fontbox/src/main/java/org/apache/fontbox/ttf/GlyphSubstitutionTable.java:
##########
@@ -799,10 +806,11 @@ public int getSubstitution(int gid, String[] scriptTags, List<String> enabledFea
}
/**
- * For a substitute-gid (obtained from {@link #getSubstitution(int, String[], List)}), retrieve the original gid.
- *
- * Only gids previously substituted by this instance can be un-substituted. If you are trying to unsubstitute before
- * you substitute, something is wrong.
+ * For a substitute-gid (obtained from {@link #getSubstitution(int, String[], List)}),
+ * retrieve the original gid.
+ * <br>
Review Comment:
Usually `<p>` is used in Javadoc to start a new paragraph.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org