search

ebiguy / RespeQt

RespeQt Atari serial peripheral emulator
GNU General Public License v2.0
31 stars 16 forks source link

Some changes to reduce the compiler warnings #47

Closed josch1710 closed 7 years ago

jzatarski commented 7 years ago

just to confirm, none of these break compilation, right? in particular, I'm looking at the parameter list changes to DiskImagePro::format and FolderImage::format.

jzatarski commented 7 years ago

Doing a quick recursive grep of the RespeQt source, format is only ever called with a DiskGeometry as the parameter. I personally don't get a warning here (probably because format(DiskGeometry) is technically implemented in diskimage.cpp), but I suspect at one time the base class changed and the derived weren't updated to follow it. I'm going to merge the request, I guess.

josch1710 commented 7 years ago

I got a warning that the format in DiskImagePro and FolderImage shadow that one on DiskImage.

jzatarski commented 7 years ago

I'd need the error copied and pasted to know what's going on for sure.

josch1710 commented 7 years ago

Was something like this: Warning: 'DiskImagePro::format' hides overloaded virtual function [-Woverloaded-virtual] bool format(int x, int y); ^

jzatarski commented 7 years ago

well, there is a reason this is just a warning. In some cases, it's not really a problem. In this case, I think there is a problem (and it needs to be corrected as you've done), but I'll have a closer look to be sure.

Basically, this warning arises ANY time a virtual function is overloaded by a derived class. I suspect this warning doesn't come up for me because I'm compiling with GCC and you're using clang.

Basically the issue here is that DiskImagePro (call it DIP for short) and FolderImage (call it FI for short) overload the format function. That is, the base class has format(DiskGeometry) defined as a virtual function, and then DIP and FI overload it with format(quint16, quint16) as a separately defined function. That is, format(DiskGeometry) refers to one function, and format(quint16, quint16) is actually an entirely different function. Technically, this is allowed, and may even be what is supposed to happen. In this case, since the format(quint64,quint64) version isn't used anywhere else, I suspect it's a mistake. As I said, I have to check to make sure though.

josch1710 commented 7 years ago

Yeah, I know all that, so I changed it ;-) I couldn't find any call to format with other parameter list, so there is no reason for the other parameter list.

jzatarski commented 7 years ago

so, my suspicion is that format(quint16,quint16) is an older version of the format function, before somebody decided they better create a DiskGeometry class to deal with disk geometries. Then somebody made that change to diskimage.h/cpp, but never bothered to update the derived classes.

jzatarski commented 7 years ago

josch, the only difference is that now any calls to format a DiskImagePro, or a FolderImage automatically return false. So, the behavior potentially changed, which is why I decided to take a closer look into it. Either way, a format of a Pro image (R/O I think) or FolderImage (not really an image at all) probably should return false, and fail a format. Maybe this happened before anyway (since likely the format() defined in SimpleDiskImage doesn't work right with the other two classes anyway).

josch1710 commented 7 years ago

I suspected that, too. Being seeing that every day on the job. Unfortunately I might add :-)