Page MenuHomePhabricator

[NFC][clang][PCH] ASTStmtReader::VisitStmt(): fixup faulty assert.
AbandonedPublic

Authored by lebedev.ri on Mar 10 2019, 3:28 PM.

Details

Summary

While this is NFC - NumStmtFields is currently 0, the assert is faulty.

It's not checking that VisitStmt() is called when getIdx() returns NumStmtFields.
It is checking that VisitStmt() is called *first*, when getIdx() returns 0.

I have stumbled into this while changing NumStmtFields to 1.

Diff Detail

Repository
rC Clang

Event Timeline

lebedev.ri created this revision.Mar 10 2019, 3:28 PM

I am not an expert in the serialization code (just did some modifications), but this seems reasonable to me.

I am not an expert in the serialization code (just did some modifications), but this seems reasonable to me.

That's my thoughts too..
Unless of course it is also testing that NumStmtFields is zero.
Anyone with more knowledged opinion?

I am not an expert in the serialization code (just did some modifications), but this seems reasonable to me.

That's my thoughts too..
Unless of course it is also testing that NumStmtFields is zero.
Anyone with more knowledged opinion?

On further though I think the assert is fine if you view it as the last thing in VisitStmt, since it is checking that you did not forget a field. Instead just move the deserialization of the new bit you introduce in D59214 so that the assert is last.

lebedev.ri abandoned this revision.Mar 15 2019, 3:09 PM

I am not an expert in the serialization code (just did some modifications), but this seems reasonable to me.

That's my thoughts too..
Unless of course it is also testing that NumStmtFields is zero.
Anyone with more knowledged opinion?

On further though I think the assert is fine if you view it as the last thing in VisitStmt, since it is checking that you did not forget a field. Instead just move the deserialization of the new bit you introduce in D59214 so that the assert is last.

Oh, that should actually be it.