Page MenuHomePhabricator

[analyzer][NFC] Refactoring BugReporter.cpp P4.: If it can be const, make it const
ClosedPublic

Authored by Szelethus on Jul 28 2019, 2:58 PM.

Details

Summary

When I'm new to a file/codebase, I personally find C++'s strong static type system to be a great aid. BugReporter.cpp is still painful to read however: function calls are made with mile long parameter lists, seemingly all of them taken with a non-const reference/pointer. This patch fixes nothing but this: make a few things const, and hammer it until it compiles.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Jul 28 2019, 2:58 PM
NoQ added a comment.EditedJul 29 2019, 6:41 PM

It's counter-idiomatic to pass manager objects (ProgramStateManager, SValBuilder, etc.) as const-references because it tells you pretty much nothing about what you can or cannot do with them. You don't really ever semantically mutate them in the first place. There's no need to mention it every time you pass them around.

But when it comes to actual data structures such as ExplodedGraph, we should totally be doing this, yeah :)

In D65382#1605572, @NoQ wrote:

It's counter-idiomatic to pass manager objects (ProgramStateManager, SValBuilder, etc.) as const-references because it tells you pretty much nothing about what you can or cannot do with them. You don't really ever semantically mutate them in the first place. There's no need to mention it every time you pass them around.

Most managers are stateful because they also store the elements they manage (e.g. ProgramStateManager stores states, SValBuilder owns other managers such as SymbolManager that stores symbols etc.) It is also true that BugReporter should not change the existing set of states, symbols etc., just use them in a read-only way. However, I see more confusion than help in making them const here and non-const elsewhere.

NoQ added a comment.Jul 31 2019, 7:59 AM

Most managers are stateful because they also store the elements they manage (e.g. ProgramStateManager stores states, SValBuilder owns other managers such as SymbolManager that stores symbols etc.)

That's an implementation detail, a memoization, a good use case for mutable if we go for const-correctness. For example, no observable properties of SymbolManager change when it allocates a new symbol. The interface only allows you to ask "What's the symbol for a value-that-transcends-time with a certain identity?" and it retrieves you that symbol. There's no way to find out if the symbol is freshly created or already cached, and there shouldn't be a way to find this out, because this is entirely immaterial and meaningless.

Szelethus updated this revision to Diff 212808.Aug 1 2019, 7:16 AM

Don't return ProgramStateManager and SValBuilder by const reference.

NoQ accepted this revision.Aug 2 2019, 6:46 PM

Do it!

This revision is now accepted and ready to land.Aug 2 2019, 6:46 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 11:48 AM