Community
Participate
Working Groups
Build ID: I20090309-0100 /** * * Invalid custom tag {@custom "Invalid"} */ public class VerificationMain { } Eclipse allows a custom tag as an inline tag, which is not allowed as per the specification. http://java.sun.com/j2se/1.4.2/docs/tooldocs/windows/javadoc.html#tag The Javadoc tool complains of an unknown tag for the given example. According to Fred, the changes made in the following fix could have been the cause. https://bugs.eclipse.org/bugs/show_bug.cgi?id=237742 More information:
Created attachment 129810 [details] Proposed patch The fix identifies all cases where a custom inline tag is used and reports an error. The createTag has the code that does the check for inline tag.
Created attachment 131000 [details] Proposed patch The changes include a lookup array in the JavadocTagConstants to mark each tag as one of TAG_TYPE_INLINE, TAG_TYPE_BLOCK and TAG_TYPE_NONE. This will be used in JavadocParser to raise warnings when an invalid tag (either custom tag or block tags) is used as an inline tag. Also, the code that handles the allowed block tags are added with a check of !this.inlineTagStarted, which ensures that we don't process tags that are not allowed anyhow.
Created attachment 131151 [details] New patch 1) always put booleans before method calls in a boolean expression e.g. instead of having the following if condition: if (length == TAG_RETURN_LENGTH && CharOperation.equals(TAG_RETURN, tagName, 0, length) && !this.inlineTagStarted) { put the test of inlineTagStarted before the method call: if (!this.inlineTagStarted && length == TAG_RETURN_LENGTH && CharOperation.equals(TAG_RETURN, tagName, 0, length)) { Then, if the boolean is true, the method will not be called and some time will be saved while running your code :-) ---------- Incorporated the suggested change. ---------- 2) the test looks good but I think you should also add test cases for all the the concerned tags: @author, @category, @deprecated, etc. ---------- Incorporated the suggested change. ---------- 3) I don't really like the fact that the array JAVADOC_TAG_TYPE is made by hand... Arrays for block and inline tags already exist (BLOCK_TAGS and INLINE_TAGS). Isn't it possible to use them to decide whether to report the problem or not? ---------- If we used the existing arrays, then we would end up iterating through atleast one array to find whether or not a tag is an inline tag. And I am sure you will agree about the performance impact of that approach. This new approach may require us to have an extra array, but is rather effective in terms of performance. Besides, it's more generic in that later if we do need to find whether a tag is a block tag, we can use it for that purpose too. ----------
(In reply to Fred's Comments) > I should have been clearer on my last point... > I agree about the performance issue if you would iterate the existing arrays > each time. My concern was more about the initialization of the new array > by hand. You can initialize the array JAVADOC_TAG_TYPE by iterating the > BLOCK_TAGS and INLINE_TAGS arrays once at the beginning, typically in a > static initializer of the JavadocParser (see one similar example in > CompletionJavadocParser...) > Cordialement/Regards, > Frédéric Yeah, I guess the following statement made me think that way. "Arrays for block and inline tags already exist (BLOCK_TAGS and INLINE_TAGS). Isn't it possible to use them to decide whether to report the problem or not? Isn't it possible to use them to decide whether to report the problem or not?" Anyhow, there are a couple of reasons why I believe it's better to keep a pre-initialized array. 1. The BLOCK_TAGS and INLINE_TAGS arrays hold the string value of the tags, which means, we will be iterating the array and do string comparison to determine each tag's type. For example, if a tag is of type TAG_TYPE_NONE, then we will know this only when we have finished iterating through both the arrays by which we would have done about 21 string comparisons. Hence, even if it is done once, I would like to avoid those hundreds of String comparisons. We don't want to slow down eclipse start-up any more, do we? :) 2. I see the merits in your suggestions that we will not have to modify the new array when there are new additions to the tags list. However, that may not happen soon and the constants defined in the JavadocTagConstants may not undergo changes. Besides even the INLINE_TAGS and BLOCK_TAGS are hand-picked and more importantly defined in the same place, it would not be such an overhead to maintain the new array also. Since we get some performance benefit out of this, I would prefer this one unless there is a chance of an error by manually populating the array, which I don't think is the case.
(In reply to comment #4) > (In reply to Fred's Comments) > > I should have been clearer on my last point... > > > I agree about the performance issue if you would iterate the existing arrays > each time. My concern was more about the initialization of the new array > by hand. You can initialize the array JAVADOC_TAG_TYPE by iterating the > BLOCK_TAGS and INLINE_TAGS arrays once at the beginning, typically in a > static initializer of the JavadocParser (see one similar example in > CompletionJavadocParser...) > > > Cordialement/Regards, > > Frédéric > > Yeah, I guess the following statement made me think that way. > > "Arrays for block and inline tags already exist (BLOCK_TAGS and INLINE_TAGS). > Isn't it possible to use them to decide whether to report the > problem or not? Isn't it possible to use them to decide whether to report the > problem or not?" > > Anyhow, there are a couple of reasons why I believe it's better to keep a > pre-initialized array. > > 1. The BLOCK_TAGS and INLINE_TAGS arrays hold the string value of the tags, > which means, we will be iterating the array and do string comparison to > determine each tag's type. For example, if a tag is of type TAG_TYPE_NONE, then > we will know this only when we have finished iterating through both the arrays > by which we would have done about 21 string comparisons. Hence, even if it is > done once, I would like to avoid those hundreds of String comparisons. We don't > want to slow down eclipse start-up any more, do we? :) > Implement it and we'll see how much time it takes... My assumption is that if this initialization is efficient enough, it should be negligible regarding to the entire time to initialize the JDT/Core plugin. > 2. I see the merits in your suggestions that we will not have to modify the new > array when there are new additions to the tags list. However, that may not > happen soon and the constants defined in the JavadocTagConstants may not > undergo changes. Besides even the INLINE_TAGS and BLOCK_TAGS are hand-picked > and more importantly defined in the same place, it would not be such an > overhead to maintain the new array also. > We never know how Javadoc tool will evolve in the future. New tags may be introduced during 1.7 and when this will happen you surely forgets to change this array and then introduce bugs... Or may be someone else will be in charge of the Javadoc area and does not know that this array needs to be modified when a new tag is added... And what about if the values for tags are changed? > Since we get some performance benefit out of this, I would prefer this one > unless there is a chance of an error by manually populating the array, which I > don't think is the case. > Arrays in JavadocTagConstants are manually created when there's no other way to do so, but in this peculiar case there's a possibility to initialize it programatically. As I said, we already walk through existing arrays to initialize constants in other parsers. So, I continue to think that it is better to do this way (of course, if the time measures confirm my assumption)...
(In reply to comment #5) > (In reply to comment #4) > > (In reply to Fred's Comments) > Implement it and we'll see how much time it takes... My assumption is that if > this initialization is efficient enough, it should be negligible regarding to > the entire time to initialize the JDT/Core plugin. I am inclined to agree that this overhead should be negligible. > We never know how Javadoc tool will evolve in the future. New tags may be > introduced during 1.7 and when this will happen you surely forgets to change > this array and then introduce bugs... Or may be someone else will be in charge > of the Javadoc area and does not know that this array needs to be modified when > a new tag is added... And what about if the values for tags are changed? The two points raised about (a) new tags in 1.7+ and (b) tag values changing for some reason are genuine and so we should try and be resilient That said, I wonder if alternately this can be guaranteed by writing a test that ensures those constraints. That way we can leave the array initialization to be simple as it is now. In this case, the programmatic initialization will be (relatively) clunky I think. Thoughts ?
Created attachment 131908 [details] Proposed patch This is the patch after incorporating two of the Fred's suggestions. This also includes the test case that checks for validity of the new array created in the JavadocTagConstants.java.
Created attachment 131909 [details] Patch with Fred's approach This is the patch incorporating Fred's suggestion for auto populating the new array of javadoc tag types. Fred, I have attached both the patches, one as per my original approach and the latter (this one) using the one mentioned by you. I would personally prefer the earlier one. However, I am leaving it to you to make the decision.
(In reply to comment #7) > Created an attachment (id=131908) [details] > Proposed patch > > This is the patch after incorporating two of the Fred's suggestions. This also > includes the test case that checks for validity of the new array created in the > JavadocTagConstants.java. > OK for this approach but the test should be more complete than the one you wrote in this patch... First, why did you write in an old Javadoc formatter test suite instead of JavadocBugsTest? Second, you currently only test the array length (I guess this is for possible tag addition), but I also think another test is necessary to ensure that indexes are still the expected ones (typically this test would fail if one or more tag values changed in the future...)
(In reply to comment #8) > Created an attachment (id=131909) [details] > Patch with Fred's approach > > This is the patch incorporating Fred's suggestion for auto populating the new > array of javadoc tag types. > > Fred, > I have attached both the patches, one as per my original approach and the > latter (this one) using the one mentioned by you. I would personally prefer > the earlier one. However, I am leaving it to you to make the decision. > Of course, this patch becomes obsolete after my previous comment, however, I take advantage of it to remember you some important points while writing patches inside JDT/Core core: 1) always try to use local variable in a loop rather than a field: instead of for (short i=0; i < BLOCK_TAGS.length; i++) { use: int length=BLOCK_TAGS.length; for (short i=0; i < length; i++) { In fact this pattern may be generalized by: avoid to call long distant fields inside a loop, use local variable instead. In your patch, tagName.length is a typical example: you access it several times in the i and j loops although you can either store this length in a local variable or better, extract the test outside the loops: E.g. char[] tagName = TAG_NAMES[index]; if (tagName.length > 0) { for (short i=0; i < BLOCK_TAGS.length; i++) { for (short j=0; j < BLOCK_TAGS[i].length; j++) { // Compare the string here ... 2) we always use an int in loop for the index. Not sure there's a performance reason for that, but when you insert code in JDT/Core, please try to use similar patterns than your predecessors. That makes the code more readable. Some additional remarks more specific to this code area: 1) lengths of BLOCK_TAGS and INLINE_TAGS were already defined as constants, so you should have use them instead of accessing the length field of these arrays. 2) in case of constants, it's sometimes possible to use identity comparison instead of equality one. So, respecting all these previous remarks, your initialization would have been faster if written as follow: static { int tagsLength = TAG_NAMES.length; nextTag:for (int index=0; index < tagsLength; index++) { char[] tagName = TAG_NAMES[index]; if (tagName.length > 0) { for (int i=0; i < BLOCK_TAGS_LENGTH; i++) { int length = BLOCK_TAGS[i].length; for (int j=0; j < length; j++) { // Compare the string here if (tagName == BLOCK_TAGS[i][j]) { JAVADOC_TAG_TYPE[index] = TAG_TYPE_BLOCK; continue nextTag; } } } for (int i=0; i < INLINE_TAGS_LENGTH; i++) { int length = INLINE_TAGS[i].length; for (int j=0; j < length; j++) { // Compare the string here if (tagName == INLINE_TAGS[i][j]) { JAVADOC_TAG_TYPE[index] = TAG_TYPE_INLINE; continue nextTag; } } } } JAVADOC_TAG_TYPE[index] = TAG_TYPE_NONE; } } Note that this snippet assumes that JAVADOC_TAG_TYPE is still a constant declared as follow in JavadocTagConstants: public final static int[] JAVADOC_TAG_TYPE = new int[TAG_NAMES.length];
(In reply to comment #9) > (In reply to comment #7) > but I also think > another test is necessary to ensure that indexes are still the expected ones > (typically this test would fail if one or more tag values changed in the > future...) > Fred, can you elaborate a bit more on this new test that you want? Did you mean, we should take each value in the JAVADOC_TAG_TYPE array and check if the value is the correct one among (TAG_TYPE_NONE=0, TAG_TYPE_INLINE=1, TAG_TYPE_BLOCK=2) manually? If we do that, then we would end up in a situation where we would have to keep updating the test cases also whenever there are new tag additions. Or did you mean, check it against the existing INLINE_TAGS and BLOCK_TAGS. Only caveat with this one is that it would be relying on INLINE_TAGS and BLOCK_TAGS, which I believe are not tested in the same was as we are trying to test the new array. Please correct me if I am wrong and if we do have test cases for INLINE_TAGS and BLOCK_TAGS already.
(In reply to comment #10) > (In reply to comment #8) > > static { > int tagsLength = TAG_NAMES.length; > nextTag:for (int index=0; index < tagsLength; index++) { > char[] tagName = TAG_NAMES[index]; > if (tagName.length > 0) { > for (int i=0; i < BLOCK_TAGS_LENGTH; i++) { > int length = BLOCK_TAGS[i].length; > for (int j=0; j < length; j++) { > // Compare the string here > if (tagName == BLOCK_TAGS[i][j]) { > JAVADOC_TAG_TYPE[index] = TAG_TYPE_BLOCK; > continue nextTag; > } > } > } > for (int i=0; i < INLINE_TAGS_LENGTH; i++) { > int length = INLINE_TAGS[i].length; > for (int j=0; j < length; j++) { > // Compare the string here > if (tagName == INLINE_TAGS[i][j]) { > JAVADOC_TAG_TYPE[index] = TAG_TYPE_INLINE; > continue nextTag; > } > } > } > } > JAVADOC_TAG_TYPE[index] = TAG_TYPE_NONE; > } > } > Fred, Indeed, highly valuable suggestions and I can't agree with more on the comparison of object references. A couple of points from me: 1. I was wondering if we can somewhow avoid the following accesses too: char[] tagName = TAG_NAMES[index]; int length = INLINE_TAGS[i].length; tagName == BLOCK_TAGS[i][j] We seem to be accessing these either inside a single or nested for loop. 2. About using short against int, the reason being using as less memory as we can under any circumstances. The code may look similar to everywhere else. But it would be using more resources than required. Anyhow, we don't have to worry about this now as we will be taking the alternative approach.
(In reply to comment #11) > > Fred, can you elaborate a bit more on this new test that you want? Did you > mean, we should take each value in the JAVADOC_TAG_TYPE array and check if the > value is the correct one among (TAG_TYPE_NONE=0, TAG_TYPE_INLINE=1, > TAG_TYPE_BLOCK=2) manually? If we do that, then we would end up in a situation > where we would have to keep updating the test cases also whenever there are new > tag additions. > This approach is not really pretty but would also work. As soon as the tag names would be modified (either with new tags added or by changing the tag values), then this test failed, hence would oblige to keep the JAVADOC_TAG_TYPE array in sync. But of course, both the array *and* the test would need to be modified... > Or did you mean, check it against the existing INLINE_TAGS and BLOCK_TAGS. > Only caveat with this one is that it would be relying on INLINE_TAGS and > BLOCK_TAGS, which I believe are not tested in the same was as we are trying > to test the new array. Please correct me if I am wrong and if we do have test > cases for INLINE_TAGS and BLOCK_TAGS already. > The goal here is not to test the INLINE_TAGS and BLOCK_TAGS arrays (how do you want to test them?) but to test that the added JAVADOC_TAG_TYPE array is still in sync with those arrays... Saying that, with this second approach, only the JAVADOC_TAG_TYPE array would need to be modified, as the test would be able to retrieve by itself the correct values for the inline and block tags. IMO, the content of the test should not be so different that the static initializer of the other patch... hence should not cost a lot to implement it. So, I would vote for the second approach...
(In reply to comment #12) > > Fred, > Indeed, highly valuable suggestions and I can't agree with more on the > comparison of object references. A couple of points from me: > > 1. I was wondering if we can somewhow avoid the following accesses too: > char[] tagName = TAG_NAMES[index]; > int length = INLINE_TAGS[i].length; > tagName == BLOCK_TAGS[i][j] > We seem to be accessing these either inside a single or nested for loop. > Not sure to understand what do you want to avoid here...? > 2. About using short against int, the reason being using as less memory as we > can under any circumstances. The code may look similar to everywhere else. But > it would be using more resources than required. > This would be true for arrays but this is wrong for single variables. A short uses two bytes, but all current machines are at least 32 bits ones, hence the minimum number of bytes they can manipulate at one time in their registers are 4 bytes (i.e. an int). Talking with Philippe about this, may lead to use the same memory (4 bytes) but will be a little bit slower, typicall while incrementing the variable to be sure that the higher bits are set to 0... So, now there are 2 reasons not to use a short in a loop... ;-) > Anyhow, we don't have to worry about this now as we will be taking the > alternative approach. > If you implement a generic test as I suggested in my previous comment, then I guess that we still need to worry about this... :-)
Created attachment 132025 [details] Latest patch Moved and modified one of the test cases as per Fred's suggestions.
(In reply to comment #15) > Created an attachment (id=132025) [details] > Latest patch > > Moved and modified one of the test cases as per Fred's suggestions. > Please just fix the weird indentation in testBug267833_3() method and that will be OK for me :-)
Created attachment 132040 [details] Latest patch Weirdly the eclipse auto formatter rearranges the indentation. Resubmitting after manually formatting the method.
Comment on attachment 132040 [details] Latest patch The patch looks good.
Comment on attachment 132040 [details] Latest patch Finally, cancel my previous as there's a minor issue in testBug267833_3: the line before * @param i is missing a trailing new line character (\n), hence the @param tag is not seen as a block tag... Please fix it and repost the patch, thx
Created attachment 132079 [details] Latest patch Incorporated Fred's latest suggestion.
Comment on attachment 132079 [details] Latest patch The patch is good for me
Released for 3.5M7 in HEAD stream.
(In reply to comment #0) > Eclipse allows a custom tag as an inline tag, which is not allowed as per the > specification. > > http://java.sun.com/j2se/1.4.2/docs/tooldocs/windows/javadoc.html#tag Sorry, I did not read all comments here, but that statement is actually wrong. You can indeed not use the -tag option to define custom inline tags, but custom inline tags are actually supported via the -taglet option, see http://java.sun.com/javase/6/docs/technotes/tools/windows/javadoc.html#taglet The Taglet Javadoc even gives an example: http://java.sun.com/javase/6/docs/jdk/api/javadoc/taglet/com/sun/tools/doclets/Taglet.html
(In reply to comment #23) > (In reply to comment #0) > > Eclipse allows a custom tag as an inline tag, which is not allowed as per the > > specification. > > > > http://java.sun.com/j2se/1.4.2/docs/tooldocs/windows/javadoc.html#tag > > Sorry, I did not read all comments here, but that statement is actually wrong. > > You can indeed not use the -tag option to define custom inline tags, but custom > inline tags are actually supported via the -taglet option, see > http://java.sun.com/javase/6/docs/technotes/tools/windows/javadoc.html#taglet > > The Taglet Javadoc even gives an example: > http://java.sun.com/javase/6/docs/jdk/api/javadoc/taglet/com/sun/tools/doclets/Taglet.html > You definitely right on that point, thanks for the smart hint Markus. My fault, I asked Jay to report this bug as I noticed warning while generating some doc including custom inline tags... :-( I'll revert the change and tag this bug as WONTFIX...
Hmmm, in fact there are two parts in the fix: 1) verify that block tag name are not used as inlined ones and vice-versa 2) verify that custom tags are not used as inline tags Second point must be removed but I'm not sure this is a good idea to revert the first one...
(In reply to comment #25) > Hmmm, in fact there are two parts in the fix: > 1) verify that block tag name are not used as inlined ones and vice-versa > 2) verify that custom tags are not used as inline tags > > Second point must be removed but I'm not sure this is a good idea to revert the > first one... > I've verified that trying to use a standard tag name in a taglet make it impossible to use while generating the javadoc documentation due to following exception: javadoc: error - Error - Exception java.lang.reflect.InvocationTargetException thrown while trying to register Taglet doclets.TestTaglet... So, Jay, please rewrite a patch from HEAD to remove warnings on custom inline tags but still verifying that standard tags are not used in the wrong category, thanks
Created attachment 133050 [details] Proposed patch Since we found that custom tags are indeed allowed for inline and block tags, removing that constraint from the previous fix and resubmitting.
(In reply to comment #27) > Created an attachment (id=133050) [details] > Proposed patch > > Since we found that custom tags are indeed allowed for inline and block tags, > removing that constraint from the previous fix and resubmitting. > Jay, you surely do not want me to release this patch like this, do you? Please: 1. remove the commented lines of code 2. remove the comment with the question about reseting tagWaitingForDescription: yes, we should reset it... as I already answered in my mail 3. add a comment in this if block to explain the rule to report the unexpected tag problem for block/inline tags (with a link to this bug) Thanks
Created attachment 133083 [details] Latest patch Removed the code that was not required and had been commented out.
(In reply to comment #29) > Created an attachment (id=133083) [details] > Latest patch > > Removed the code that was not required and had been commented out. > Thanks for the changes, but as I said in my comment, a link reference would be better for the comment rather than the bug summary, hence one can directly jump to bugzilla from the code using Ctrl+mouse left button click... E.g.: // Report an 'Unexpected tag' problem when a standard tag is being used // in the wrong context // (see bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=267833)
Created attachment 133106 [details] Latest patch Modifying the comments and bug reference the way Fred wants.
(In reply to comment #31) > Created an attachment (id=133106) [details] > Latest patch > Released for 3.5M7 in HEAD stream.
Verified for 3.5M7 using I20090426-2000
