Clean up uses of SsaNames in the code base

google-research / raksha

Apache License 2.0

35 stars 17 forks source link

Clean up uses of SsaNames in the code base #615

Open bgogul opened 2 years ago

bgogul commented 2 years ago

Other than the place where SsaNames is constructed, we should not have to pass SsaNames as a non-const value. The following changes should make it possible:

Change GetOrCreateID to GetID that returns ID if it is available or returns an error.
IRPRinter should not hold on to a std::unique_ptr<SsaNames>. Instead, it should hold a const SsaNames* passed to it through a constructor.
Remove Value::ToString and replace it with IRPrinter::ToString(Value v, const SsaNames* ssa_names).

This is just a start, but more changes may be needed.

harsha-mandadi commented 2 years ago

Also, add ToString to Operation for ease of degubbing!!

bgogul commented 2 years ago

Also, add ToString to Operation for ease of degubbing!!

Instead of adding it directly to Operation, wouldn't it be better to a IRPrinter::ToString(const Operation&, ...). Otherwise, don't you need to pass SsaNames to Operation and would introduce cyclic dependency again?

bgogul commented 2 years ago

Just some thoughts based on discussion in meeting that need to be vetted further:

May be it is OK to leave GetOrCreateID and add a GetID method.
We might want to make SsaNames to be part of IrContext.

harsha-mandadi commented 2 years ago

Similar to IRPrinter related changes, datalog_lowering_visitor used to lower datalog to facts in //src/backends/policy_engine/souffle/ should not populate ssa_names. It should be passed to it through constructor.