Jens-G commented on PR #3666:
URL: https://github.com/apache/thrift/pull/3666#issuecomment-5567271509
### Code review
Re-reviewed at `d8ec9325e`. All seven issues and the three suggestions from
the last round are fixed, and the design point landed as the
`get_doc_type_name()` hook — the parallel `@throws` loops are down from four to
three (the shared one, netstd's, which has to build a different element, and
php's pre-existing one). Everything below was reproduced against compilers
built from the PR head and from the merge base (`3dece7e9e`). The netstd output
was compiled with `GenerateDocumentationFile=true`: 0 warnings, and every
member survives into the `.xml` — the `cref` values resolve as
`T:Main.Ns.object` / `T:Main.Ns.native`.
Found 1 issue:
1. JavaME doubles the package for an exception declared in an included file.
`type_name()` already qualifies a type whose program is not the one being
generated, so prefixing `get_namespace("java")` on top of it yields a name no
class has. With `inc.thrift` (`namespace java com.inc`, `exception RealExc`)
included from `main.thrift`, the generated interface contradicts itself on
adjacent lines:
```java
/**
* doc
*
* @throws com.inc.com.inc.RealExc
*/
public void viaInclude() throws com.inc.RealExc, TException;
```
Both the direct-include and the typedef path hit it; a same-file
exception is unaffected. `t_java_generator::get_doc_type_name()` avoids this by
letting `type_name(..., force_namespace = true)` qualify once — javame's
`type_name()` has no such parameter, so the extra prefix needs to be
conditional on the type being local.
https://github.com/apache/thrift/blob/d8ec9325e8bbf9f6b8bb505e912bd2bcf9445eae/compiler/cpp/src/thrift/generate/t_javame_generator.cc#L2837-L2843
The qualification it duplicates:
https://github.com/apache/thrift/blob/d8ec9325e8bbf9f6b8bb505e912bd2bcf9445eae/compiler/cpp/src/thrift/generate/t_javame_generator.cc#L2825-L2835
Three suggestions, below the bar for the list above but verified:
- `exception_doc_test.py` covers five of the seven languages in the
`Client:` trailer — py and javame are missing, and javame is where the issue
above lives. Both fit the existing harness unchanged (`--gen py` writes a
`Raises:` section, `--gen javame` writes `@throws`), so it is two more
`_generate()` calls.
- The new `t_netstd_generator::xml_encode()` is the same three replacements
as
[`t_delphi_generator::xml_encode()`](https://github.com/apache/thrift/blob/d8ec9325e8bbf9f6b8bb505e912bd2bcf9445eae/compiler/cpp/src/thrift/generate/t_delphi_generator.cc#L472-L487).
Both classes derive from `t_oop_generator`, which is already where
`replace_all()` lives, so one copy there would serve both.
- A method whose exceptions are documented but which carries no doc of its
own opens its comment with an empty line, because `generate_throws_doc()`
unconditionally leads with `"\n@throws"`. cpp, java, javame, kotlin, haxe and
netstd all emit `/**\n * \n * @throws …`. Cosmetic, but it lands on exactly the
case this round made work.
https://github.com/apache/thrift/blob/d8ec9325e8bbf9f6b8bb505e912bd2bcf9445eae/compiler/cpp/src/thrift/generate/t_oop_generator.h#L44-L57
On scope: delphi does generate method documentation — `--gen delphi:xmldoc`
emits `<summary>` and `<param>` from [`generate_delphi_doc(ostream&,
t_function*)`](https://github.com/apache/thrift/blob/d8ec9325e8bbf9f6b8bb505e912bd2bcf9445eae/compiler/cpp/src/thrift/generate/t_delphi_generator.cc#L536-L559)
— and is not in the `Client:` list. Same shape as the netstd path. Not a
blocker here; a follow-up ticket would be fine.
🤖 Generated with [Claude Code](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react
with 👎.</sub>
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
