clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.
================
Comment at: include/lldb/Core/Architecture.h:119-123
+ virtual std::vector<ConstString>
+ ConfigurationRegisterNames() const { return {}; }
+
+ using ConfigurationRegisterValues = std::map<ConstString, uint32_t>;
+ virtual bool SetFlagsFrom(const ConfigurationRegisterValues &) { return
true; }
----------------
Any reason these definitions need to be in this class? Seems like something the
plug-ins that access these should do, and this code shouldn't be in this file.
================
Comment at: include/lldb/Core/Architecture.h:125
+
+ virtual bool TestFlag(Flags::ValueType bit) const { return false; }
+
----------------
We should add a "Flags m_flags" as an instance variable and just provide
accessors:
```
class Architecture {
Flags GetFlags() { return m_flags; }
const Flags GetFlags() const { return m_flags; }
};
```
================
Comment at: include/lldb/Core/Architecture.h:127
+
+ virtual bool MatchFlags(const ArchSpec &spec) const { return true; }
+
----------------
It is not clear what this function does and it doesn't seem like a function
that belongs on this class.
================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:44-71
+bool ArchitectureArc::SetFlagsFrom(const ConfigurationRegisterValues ®s) {
+ ConstString rf_build("rf_build");
+ auto it = regs.find(rf_build);
+ if (it == regs.end())
+ return false;
+
+ // RF_BUILD "Number of Entries" bit.
----------------
This entire function's contents should be placed where it is called and not in
the Architecture class.
================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:73-75
+bool ArchitectureArc::TestFlag(Flags::ValueType bit) const {
+ return m_flags.Test(bit);
+}
----------------
Remove and use accessor to m_flags.
================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:77-79
+std::vector<ConstString> ArchitectureArc::ConfigurationRegisterNames() const {
+ return { ConstString("rf_build"), ConstString("isa_config") };
+}
----------------
Remove from this class and inline at call site.
================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:81-86
+bool ArchitectureArc::MatchFlags(const ArchSpec &spec) const {
+ return ((spec.GetByteOrder() == eByteOrderBig) ==
+ m_flags.Test(arc_features::big_endian)) &&
+ ((spec.GetAddressByteSize() == 4) ==
+ m_flags.Test(arc_features::address_size_32));
+}
----------------
Remove from this class and inline at call site.
================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.h:36-42
+ std::vector<ConstString> ConfigurationRegisterNames() const override;
+
+ bool SetFlagsFrom(const ConfigurationRegisterValues &) override;
+
+ bool TestFlag(Flags::ValueType bit) const override;
+
+ bool MatchFlags(const ArchSpec &spec) const override;
----------------
These four functions don't seem like they belong on the Architecture class. It
would be best to just have the code that calls these methods inline the code
where needed.
================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.h:48
+
+ lldb_private::Flags m_flags;
+};
----------------
I would put this in the base class and then add just an accessor:
```
class Architecture {
Flags GetFlags() { return m_flags; }
const Flags GetFlags() const { return m_flags; }
};
```
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55718/new/
https://reviews.llvm.org/D55718
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
