Fixes use of wrong identifier when pretty printing bitfield types #1676
Conversation
|
Fix looks good; any chance of a regression test to go with it? Preferably the one / a version of the one submitted in the original ticket. |
|
I might be responsible for a some of this code. May I ask for a slightly bigger refactoring to use pointer_offset_bits instead? All those |
|
@tautschnig I'm not sure how you'd want to use pointer_offset_bits here. Also, what would you want to replace the get_string calls with? |
|
|
@martin-cs I didn't find a test category that really fit for the test I was doing (though if there is one I'll happy to change this), so I made my own @tautschnig I'm not entirely sure if I understood you correctly, is this what you had in mind? |
hannes-steffenhagen-diffblue
force-pushed
the
goto-analyzer-develop_fix-bitfield
branch
2 times, most recently
from
fc95904 to
2b424a1
Compare
regression/Makefile
Outdated
| @@ -10,6 +10,7 @@ DIRS = ansi-c \ | |||
| goto-instrument \ | |||
| goto-instrument-typedef \ | |||
| test-script \ | |||
| ansi-c-typename \ | |||
There was a problem hiding this comment.
I'd just add it to the goto-instrument directory.
There was a problem hiding this comment.
done
src/ansi-c/type2name.cpp
Outdated
| mp_integer bits = pointer_offset_bits(type, ns); | ||
| CHECK_RETURN(bits != -1); | ||
| return integer2string(bits); | ||
| }; |
There was a problem hiding this comment.
Is there a strong reason not to use a function at file scope?
There was a problem hiding this comment.
Not really. I can do that if you prefer; I just put it in the function scope because I wasn't expecting to use it outside of the function and I don't need to pass parameters this way.
There was a problem hiding this comment.
My reasons for preferring the file-scoped function: 1) Consistency: we don't do it elsewhere. 2) I'm not sure how well doxygen can cope with it. 3) The function calls do not make clear what is captured, thus making the code harder to read.
None of this is a blocker, but readability is a very important aspect.
src/ansi-c/type2name.cpp
Outdated
| @@ -168,7 +177,7 @@ static std::string type2name( | |||
| const array_typet &t=to_array_type(type); | |||
| mp_integer size; | |||
| if(t.size().id()==ID_symbol) | |||
| result+="ARR"+t.size().get_string(ID_identifier); | |||
| result+="ARR"+id2string(t.size().id()); | |||
There was a problem hiding this comment.
No, that would yield "symbol". Use id2string(t.size().get(ID_identifier))
There was a problem hiding this comment.
done
src/ansi-c/type2name.cpp
Outdated
| @@ -202,7 +211,9 @@ static std::string type2name( | |||
| if(it!=components.begin()) | |||
| result+='|'; | |||
| result+=type2name(it->type(), ns, symbol_number); | |||
| result+="'"+it->get_string(ID_name)+"'"; | |||
| irep_idt component_name = it->get_name(); | |||
| CHECK_RETURN(component_name != ID_nil); | |||
There was a problem hiding this comment.
nil would actually be a valid name! component_name.empty() would be the error you are looking for.
There was a problem hiding this comment.
done
hannes-steffenhagen-diffblue
force-pushed
the
goto-analyzer-develop_fix-bitfield
branch
from
2b424a1 to
06a353a
Compare
| -- | ||
|
|
||
| Checks that type2name generates the right names for structs containing bitfields, | ||
| in particular including the width of bitfield fields. |
There was a problem hiding this comment.
Nit pick: no new line at end-of-file
|
(reminder: both AppVeyor and travis report failures for known issues that are unrelated to this PR) |
hannes-steffenhagen-diffblue
force-pushed
the
goto-analyzer-develop_fix-bitfield
branch
from
06a353a to
defda7b
Compare
hannes-steffenhagen-diffblue
deleted the
goto-analyzer-develop_fix-bitfield
branch
The width of a bitfield is indicated with
ID_width. Old version oftype2namewas usingID_sizeinstead, which (silently) threw away the information about the width of a bitfield in the pretty printed type.As a side note, I'm not sure it's such a good idea for the get* type functions to fail silently.