[PATCH] D138859: [ODRHash] Drive attribute hashing through TableGen. NFC intended.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D138859#3955806 <https://reviews.llvm.org/D138859#3955806>, @ChuanqiXu wrote:

> And I think we should add tests for this. e.g., in the ASTImporterTests.cpp.

I'm not sure which test would be useful in this case. We kinda already test that different modules agree which attributes should be ODR hashable. That is enforced by detecting attribute mismatches in odr_hash.cpp (if attributes aren't hashable - no mismatches).



================
Comment at: clang/include/clang/Basic/Attr.td:552-553
   bit LateParsed = 0;
+  // Set to true for attributes that should participate in ODR hashing.
+  bit IsODRHashable = 0;
   // Set to false to prevent an attribute from being propagated from a template
----------------
aaron.ballman wrote:
> Do we want to change the default for any of the derived classes? e.g., should something that is a `TypeAttr` or `DeclOrTypeAttr` default to being ODR hashable because they impact the type system?
> 
> Also, can we expand the comment somewhat to help folks understand the circumstances under which they should set that to 1?
At this point I don't know what the default value should be, for that I'll need to see how many attributes should be hashable. This change is mostly to prove the direction is viable and reasonable. And TypeAttr, DeclOrTypeAttr is a good starting point to decide which attributes should be hashed.

Good idea with expanding the comment. Though I need some time to prepare non-abhorrent explanation (gosh, docs are harder than code).


================
Comment at: clang/lib/AST/AttrImpl.cpp:16
 #include "clang/AST/Expr.h"
+#include "clang/AST/ODRHash.h"
 #include "clang/AST/Type.h"
----------------
ChuanqiXu wrote:
> Is this change necessary?
Yep. AttrImpl.cpp includes "clang/AST/AttrImpl.inc" where, for example, we have
```lang=c++
void AlignValueAttr::processODRHash(
    llvm::FoldingSetNodeID &ID, ODRHash &Hash) const {
  ID.AddInteger(Attr::getKind());
  const Expr *AlignmentExpr = getAlignment();
  ID.AddBoolean(AlignmentExpr);
  if (AlignmentExpr)
    Hash.AddStmt(AlignmentExpr);
}
```
and we need the full declaration of `ODRHash` to call `ODRHash::AddStmt`.


================
Comment at: clang/lib/AST/AttrImpl.cpp:242
 
 #include "clang/AST/AttrImpl.inc"
----------------
AttrImpl.inc include I've mentioned.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138859/new/

https://reviews.llvm.org/D138859