Conversation
|
let's run TC tests |
|
let's run TC tests |
|
let's run TC tests |
PeterMatula
left a comment
PeterMatula
left a comment
There was a problem hiding this comment.
This is too complex for me to actually review in detail. So if the tests are passing (I fixed doxygen) then so be it...
I went over it as much as I could and have only one question in the comments below, other than that I would merge it as it is.
|
|
||
| // Set FirstThunk to point into IAT | ||
| int fileIndex = _peFile->impDir().getFileIndex(moduleName, PeLib::NEWDIR); | ||
| int fileIndex = _peFile->impDir().getFileIndex(moduleName, true); |
There was a problem hiding this comment.
The original currdir enum maybe wasn't the prettiest, but I still think the code:
getFileIndex(moduleName, PeLib::NEWDIR)
getFileIndex(moduleName, PeLib::OLDDIR)was more telling then:
getFileIndex(moduleName, true)
getFileIndex(moduleName, false)Why was this changed? Do you just like it better, or was there some other reason?
There was a problem hiding this comment.
A presence of an enum indicated that it could be more values, whilst it's only decision between an old import directory or a new one. Hence I changed it to true vs false.
There was a problem hiding this comment.
In this case, I think the enum could have just served to make the code more human-readable, without any implications/expectations on how many values there are.
There was a problem hiding this comment.
However, this might be just my personal preference and if you like it as it is now, ok. Don't refactor it back unless you think it would be better.
There was a problem hiding this comment.
I agree with @PeterMatula here. When i see this code what does true really mean? When I see NEWDIR it gives me at least some sort of semantics of what is going on.
There was a problem hiding this comment.
You are missing the point. I don't want to read the signature of every single function when I'm trying to understand the code. Imagine we need additional functionality which needs another flag. What is better? Having getFileIndex(moduleName, true, false)? Wwhat does each of them mean? I don't know. So I check the signature. I come back tomorrow and I don't remember which one is which so I need to check the signature again. The best thing would even be if those flags were strongly typed (enum class) because if I want to call this function I need to exactly know which bool is which and compiler won't help me if I make a mistake. If you turn all your bool arguments into strongly-typed arguments then you have the compiler to save you when you by accident pass the wrong value into the parameter. Overall, bool arguments are bad.
There was a problem hiding this comment.
Surely we could discuss coding rules ad libitum.
Do you want me to return the enum?
|
I have more questions in the related tests PR - avast/retdec-regression-tests#107 |
|
let's run TC tests |
3 participants
No description provided.