Jens-G commented on PR #3666: URL: https://github.com/apache/thrift/pull/3666#issuecomment-5085562521
### Code review Found 5 issues: 1. Deleting javame's `generate_java_doc(ostream&, t_function*)` override silently regresses javame output. `t_javame_generator` still declares two other overloads of that name and has no `using t_oop_generator::generate_java_doc;`, so C++ name hiding removes the base `t_function*` overload from the candidate set. Since `t_function : public t_doc`, the call sites at [lines 1956](https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/generate/t_javame_generator.cc#L1955-L1957) and 1981 now bind to the `t_doc*` overload instead. Javame loses the auto-generated `@param` tags it emitted before this PR and never gains the new `@throws` tags. It compiles without a warning. https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/generate/t_javame_generator.cc#L188-L192 Worth fixing together with this: [javame's `get_namespace()`](https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/generate/t_javame_generator.cc#L253-L257) appends no trailing separator, unlike the base default (`"."`) and the cpp override (`"::"`), while the call site concatenates straight onto the type name. Once the overload resolution above is corrected, javame will emit `@throws thrift.testXception`. [Kotlin's override](https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/generate/t_kotlin_generator.cc#L183-L191) has the same shape. 2. The unconditional `ss << '\n';` runs whether or not the function declares any exceptions, and the `@throws` loop below already prefixes each entry with `\n`. Functions with no `throws()` clause get a trailing blank ` * ` line; functions with one get a doubled blank line. This affects every documented service function emitted by the cpp and java generators, which are the two that reach this shared method. Guarding it with `if (!exceptions.empty())` fixes both cases. https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/generate/t_oop_generator.h#L112-L121 3. `t_type::get_name()` returns an empty string for inline container types, because `t_list`/`t_set`/`t_map` construct through `t_container()` into the default `t_type()` ctor, which never sets `name_`. A field declared `1: list<string> items` renders as `- items ()`. Base types also come out as IDL names (`i32`, `string`) rather than Python ones. The file already has `type_to_py_type()` for exactly this, used by `arg_hint`/`member_hint`/`func_hint`, which renders `list[str]` and `dict[str, int]` correctly. https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/generate/t_py_generator.cc#L2774-L2780 4. The `@throws` name is taken from `e->get_type()` without resolving typedefs. `validate_throws()` in `main.cc` checks the thrown type via `get_true_type()`, so `typedef SomeException Alias` followed by `throws (1: Alias e)` is legal IDL, and this would emit `@throws ns.Alias` — a name with no generated class behind it, since typedefs produce no class in Java or C++. The sibling `generate_java_doc(ostream&, t_field*)` two functions above already goes through `get_true_type()` for the same reason (THRIFT-4086, `4f63573f5`), and the netstd path in this PR gets it right via `type_name()`. https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/generate/t_oop_generator.h#L118-L122 5. This change to `lib/cpp/test/TTransportFactoryConfigTest.cpp` is unrelated to THRIFT-6108 and is a no-op — it extracts `TConfiguration::DEFAULT_MAX_MESSAGE_SIZE` into a local and passes that instead of the constant. Worth dropping so the PR stays scoped to the doc generators. https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/lib/cpp/test/TTransportFactoryConfigTest.cpp#L106-L109 On the design, separately from the defects above: `get_gen_name()` looks like more machinery than the feature needs. https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/generate/t_oop_generator.h#L39-L45 Making it pure virtual obliges all 18 `t_oop_generator` subclasses to implement it, each with its own `gen_name_` member, which is most of why this PR touches 21 generator files when the machinery only ever serves the shared doc path. It has three call sites and all of them are inside `get_namespace()`, whose only consumer is the new `@throws` line — reachable from cpp, java and javame alone. Since javame's override hardcodes `"java"`, only the cpp and java implementations are ever actually called. The bigger issue is that the abstraction doesn't fit its users: four of the six `get_namespace()` overrides discard `get_gen_name()` outright — javame hardcodes `"java"`, c_glib returns `nspace`, and perl and php delegate to the existing `perl_namespace()`/`php_namespace()` helpers. It is only usable where a generator's registration name and its IDL namespace key happen to coincide, which is exactly the cpp and java case. That string also already exists in the compiler, in one place. `THRIFT_REGISTER_GENERATOR` registers each generator under `#language`: https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/generate/t_generator_registry.h#L96-L104 and `t_program::set_namespace()` validates every `namespace <key>` directive against that same map: https://github.com/apache/thrift/blob/b3754d44fe4bd6edce787f481bcf4c7ff8b95e95/compiler/cpp/src/thrift/parse/t_program.h#L297-L306 Keeping `get_namespace()` as the virtual hook and dropping `get_gen_name()` entirely would give the same result: `t_cpp_generator::get_namespace()` passes `"cpp"` directly, the base default passes `"java"`, and the other 16 generators need no change at all. That is the pattern the rest of the compiler already uses, e.g. `t_netstd_generator.cc:140` and `t_kotlin_generator.cc:202`. 🤖 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]
