Details
Diff Detail
- Repository
- rC Clang
Event Timeline
| include/clang/AST/AttrVisitor.h | ||
|---|---|---|
| 22 | I don't think the namespace adds value. | |
| 24–25 | Oh, this is cute. We have StmtVisitor which does not use a namespace, but defines these functions. Then we duplicate these function definitions in each of the comment, decl, and attribute visitors under distinct namespaces so we don't get ODR violations. I think we should add llvm::make_const_ptr to STLExtras.h so we can use the same definition from everywhere. We can use std::add_pointer in place of make_ptr, because that is a one-to-one mapping, so we can drop make_ptr entirely. However, I don't think we can use std::add_pointer<std::add_const> similarly because there's no way to bind that to the template template parameter directly. I'm happy to make these changes if you'd like. | |
| 28 | s/class/typename | |
| include/clang/AST/TextNodeDumper.h | ||
| 186 | I'd appreciate some comments gently reminding the reader that this include introduces declarations which are expected to be members of a class. | |
| lib/AST/ASTDumper.cpp | ||
| 443 | Similarly, add some comments about how this adds implementations to the class. | |
| utils/TableGen/ClangAttrEmitter.cpp | ||
| 3714 | FunctionContent | |
| 3725–3759 | FunctionContent | |
| 3726 | Wouldn't this be !FunctionContent.empty()? | |
| include/clang/AST/AttrVisitor.h | ||
|---|---|---|
| 22 | I think the point is to separate the implementation detail. I don't care either way, but the other visitors should be changed first and this one can follow the same pattern. | |
| 24–25 | Yes, I've thought about this duplication too. If you deduplicate, I'll rebase this change. | |
| 28 | I think c++17 is the first to allow typename here. | |
| include/clang/AST/AttrVisitor.h | ||
|---|---|---|
| 24–25 | r348729 (LLVM) and r348730 (Clang). | |
| utils/TableGen/ClangAttrEmitter.cpp | ||
|---|---|---|
| 3726 | I would have thought so, but apparently that's not how raw_string_ostream works. I get an empty file from this if I do that. | |
| include/clang/AST/TextNodeDumper.h | ||
|---|---|---|
| 187 | Add a full stop to the end of the comment. | |
| lib/AST/ASTDumper.cpp | ||
| 89 | This seems unrelated to this patch (same below) ? | |
| 443 | Full stop here as well. | |
| lib/AST/TextNodeDumper.cpp | ||
| 44–45 | Formatting is incorrect. | |
| utils/TableGen/ClangAttrEmitter.cpp | ||
| 3726 | Huh, good to know. | |
| 3738 | The source file comment will be incorrect. This emits the node traverser not the dumper. | |
| lib/AST/ASTDumper.cpp | ||
|---|---|---|
| 89 | These were called from the code which tablegen used to generate for dumping attrs. They're no longer needed after this change. | |
| lib/AST/TextNodeDumper.cpp | ||
|---|---|---|
| 44–45 | Note that this is a scope for the color, not the opening of the function. | |
I don't think the namespace adds value.