Page MenuHomePhabricator

[AST] Refactor propagation of dependency bits. NFC
Needs ReviewPublic

Authored by ilya-biryukov on Fri, Dec 27, 1:22 AM.

Details

Summary

This changes introduces an enum to represent dependencies as a bitmask
and extract common patterns from code that computes dependency bits into
helper functions.

Diff Detail

Event Timeline

ilya-biryukov created this revision.Fri, Dec 27, 1:22 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

@rsmith, could you please take a look and let me know whether you think adding a new type for this makes sense?

On one hand, I feel it's good to have a type to represent "dependencies" and it allow to write helper functions and easily add new bits into the dependencies without rewriting all the code propagating those flags (error bit).

OTOH, using this for some things might be confusing:

  • a type cannot be value-dependent, but dependency flags allow for that.
  • a template argument can currently be "dependent" after can be either type- or value-dependent, also confusing.
  • ...

Note there are still some rough edges here, see the added FIXMEs.

Also note that this change does not move any code around to make sure this change is easy to review and validate that the code is doing the same thing.
I'm also planning to move all the code that computes dependencies into one place (it's currently scattered around many files and it certainly makes it quite hard to find bugs in the code and update it). But I would really like to get everyone aligned on the direction we're taking here.

ilya-biryukov marked an inline comment as done.Fri, Dec 27, 1:33 AM
ilya-biryukov added inline comments.
clang-tools-extra/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
17
NOTE: We need this to fix compiler errors down below. There's an AST matcher called isTypeDependent and we add a function clang::isTypeDependent(DependencyFlags) in this change.

Overload resolution does its job, but we have to make sure they're in the same namespace, so we need to put using namespace somewhere inside the clang namespace.

Unit tests: pass. 61128 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

I like the goal of this patch and the simplifications it does. I don't feel qualified to do a full review.

clang/include/clang/AST/Expr.h
126

Just curious why do you prefer D = D | DependencyFlags::Type; over D |= DependencyFlags::Type; ? The latter seems to be more common.

clang/include/clang/AST/Stmt.h
323

Please use enum { NumExprBits = NumStmtBits + 5 + DependencyFlagsBits }; to avoid bugs when the size changes.

  • Use DependencyFlagsBits for computing NumExprBits
  • Reformat
ilya-biryukov marked 4 inline comments as done.Thu, Jan 2, 2:10 AM
ilya-biryukov added inline comments.
clang/include/clang/AST/Expr.h
126

Would also prefer D |=, but it leads to compilation errors.

The builtin operator |= accepts ints and, therefore, fails on strongly-typed enums.
And, AFAIK, there's no way to redefine operator |= for non-class types.

clang/include/clang/AST/Stmt.h
323

Many thanks! Using it here was the reason I wanted to add the constant in the first place. Ended up forgetting about it.
Thanks!

Unit tests: pass. 61158 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

riccibruno added inline comments.Thu, Jan 2, 7:39 AM
clang/include/clang/AST/Expr.h
126

You certainly can define a compound assignment operator for an enumeration type. It is only non-compound-assignment that is special cased and required to be a member function.

Example: https://godbolt.org/z/JV5uPw

134

You may want to add an assertion here to make sure that no truncation occurred.

clang/lib/Serialization/ASTReaderStmt.cpp
530

I think it would be nicer to add an abbreviation for the right number of bits (using DependencyFlagsBits) and as you say just serialize/deserialize all the flags in one go.

ilya-biryukov marked 4 inline comments as done.Thu, Jan 2, 8:06 AM
ilya-biryukov added inline comments.
clang/include/clang/AST/Expr.h
126

Ah, thanks! So it turns out I was wrong. Will update the patch.

clang/lib/Serialization/ASTReaderStmt.cpp
530

Will do this in a follow-up.
That would be a functional change, I'm aiming to keep this one an NFC.

martong resigned from this revision.Tue, Jan 7, 7:37 AM

@rsmith, gentle ping. WDYT? Is this a step in the right direction?

  • Add compound assignment operations

Unit tests: pass. 61745 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml