Improved typedef handling #85
Conversation
| // seed. It also serves as the combiner the other routine variant uses to generate its new | ||
| // seed. | ||
| return (seed ^ x) + 0x9e3779b9 + (seed << 6) + (seed >> 2); | ||
| return seed ^ (x + 0x9e3779b9 + (seed << 6) + (seed >> 2)); |
|
|
||
| std::string to_json(const std::vector<odrv_report>&); | ||
|
|
||
| std::string version_json(); |
There was a problem hiding this comment.
I put this routine inside orc.cpp to encapsulate the usage of nlohmann::json to just that file.
| // attributes by name to make sure the hashing is consistent regardless of attribute order. | ||
| constexpr const std::size_t max_names_k{32}; | ||
| std::array<dw::at, max_names_k> names{dw::at::none}; | ||
| // It is fixed to keep allocations from happening. |
There was a problem hiding this comment.
This is the start of some cleanup so I could better understand what fatal_attribute_hash was doing.
| result = empool("const " + recurse(attributes).allocate_string()); | ||
| } else if (die._tag == dw::tag::pointer_type) { | ||
| result = empool(recurse(attributes).allocate_string() + "*"); | ||
| } else if (die._tag == dw::tag::typedef_) { |
There was a problem hiding this comment.
This is the major fix- an improvement to the way anonymous typedefs are handled. I think this resolves #84, but I'd like to add a test before declaring victory here. Regardless, this is the fix that addressed what Brandon found.
There was a problem hiding this comment.
Update: This is not the fix for 84.
| s << " version: " << ORC_VERSION_STR() << tag_url << '\n'; | ||
| s << " sha: " << ORC_SHA_STR() << '\n'; | ||
| }); | ||
| if (settings::instance()._output_file_mode == settings::output_file_mode::json) { |
There was a problem hiding this comment.
When the output file mode is set to json and ORC finds no files to process, it will emit a small JSON blob consisting of the ORC version and hash. This is useful for test harness wrappers that may use ORC, to know what version of the tool they are dealing with. (We now have one inside of Adobe that relies on this.)
leethomason
left a comment
leethomason
left a comment
There was a problem hiding this comment.
Looks good - one small nit / question, but looks fine.
| // seed. It also serves as the combiner the other routine variant uses to generate its new | ||
| // seed. | ||
| return (seed ^ x) + 0x9e3779b9 + (seed << 6) + (seed >> 2); | ||
| return seed ^ (x + 0x9e3779b9 + (seed << 6) + (seed >> 2)); |
src/dwarf.cpp
Outdated
| constexpr const std::size_t max_names_k{32}; | ||
| std::array<dw::at, max_names_k> names{dw::at::none}; | ||
| // It is fixed to keep allocations from happening. | ||
| constexpr const std::size_t max_names_k{32}; |
There was a problem hiding this comment.
nit pick but fascinating: is there meaning to a const constexpr? And did you mean static constexpr?
There was a problem hiding this comment.
constexpr const is redundant, yeah. I have removed the const.
And great question about static constexpr. I looked into this a bit, and it would make sense to add the static qualification if the constexpr expression needs a runtime instance (e.g., to take its address when passing it to a function.) The static in that case ensures only one such instance is created (and in a threadsafe way). Given that max_names_k is not used like that, just constexpr should suffice (though I don't think there's any harm in adding the static).
Brandon caught a case where newer versions of ORC were passing over ODRVs it was previously reporting. This fix resolves it.
This change also includes a fix for #84, which required some juggling of a name from a named typedef to an unnamed struct that immediately follows it.