MaskRay wrote: I am in favor of this change, but please can you trim the comments? I don't think it's worth mentioning this new large-eh-encoding in the comments: it's probably one of the least used options in the wild.
``` ● Yes — this has a fairly strong AI smell, mostly from verbosity + duplication rather than outright wrong content. Here's the breakdown. The tells 1. Near-verbatim duplication of a 5-line block across files. The "register flags" comment and the "seed" comment are each copy-pasted between BackendUtil.cpp and cc1as_main.cpp with only token swaps (clang's -mllvm → -cc1as -mllvm, llc, llvm-mc → llvm-mc). The same explanation then appears a third time in the test-file header and a fourth time in the PR body. Four restatements of one idea is a classic generated-text signature. 2. Comment-to-code ratio is inverted. The actual change is two lines per file (one static decl, one init). Each is wrapped in a 4–5 line comment — so ~4 comment lines per code line. LLVM convention here would be one line. 3. Restating the obvious no-op case. Every seed comment ends with: ▎ // Absent any -mllvm flags this is equivalent to a default-constructed MCTargetOptions. That's filler — InitMCTargetOptionsFromFlags() with no flags obviously yields defaults. It reads like padding to "cover all cases." 4. Explaining adjacent/other code. - "Clang's own driver-derived settings below overwrite whatever they cover" — narrates the lines that follow. - "whose accessors require the flags to have been registered" — narrates InitMCTargetOptionsFromFlags's internals. This is the "multi-line comment explaining what other code does" pattern you named. 5. Test file re-explains what inline RUN comments already say. The 8-line header block spells out the precedence model, then each RUN group re-explains the same precedence for its specific case. The per-case comments are actually the useful ones; the header is redundant. What's legitimately warranted Not all of it is slop. A bare static llvm::mc::RegisterMCTargetOptionsFlags MCTargetOptionsFlags; genuinely looks like dead code, so one line saying "register MC flags so -mllvm can reach them (as llc/llvm-mc do)" is justified. And the precedence point (clang flags win over -mllvm) is the real non-obvious design decision worth stating — once. Tightened version // Register MC target-option flags (e.g. -large-eh-encoding) so they're // reachable via -mllvm, as llc/llvm-mc do. static llvm::mc::RegisterMCTargetOptionsFlags MCTargetOptionsFlags; // Seed from -mllvm first; clang's own flags below take precedence. Options.MCOptions = llvm::mc::InitMCTargetOptionsFromFlags(); That preserves every fact a reader needs and drops ~15 lines of restatement. So: yes, strong smell — driven by duplication and over-explanation, fixable by collapsing each block to one line. ``` https://github.com/llvm/llvm-project/pull/210087 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
