| T O P I C R E V I E W |
| zzdev10 |
Posted - Oct 19 2017 : 8:44:57 PM
Hello,
I am working in a legacy code base with huge monolithic C++ classes, which are more like libraries than classes. I tried using code inspection and associated quick fixes to clean up the code a bit, but I am having troubles with the use of "override". Specifically, not all of the cases of missing "override" are detected and I have to examine each case of non-replacement manually.
I tried to isolate the test scenario, but I was not successful. So, let me describe what I am seeing and I hope that your follow up questions will help me narrow down the issue.
We have an interface definition like following:
DECLARE_INTERFACE_(IMyWidget, IUnknown)
{
// here we have a mix of type definitions and 220 pure virtual functions
};
The implementation elsewhere is defined as following:
class CMyWidget : public IMyWidget
{
// a list of functions that implement IMyWidget
}
The problem is that about 20% of the overridden functions were not detected as missing an 'override'. I initially suspected that this only happened on functions that had a 3rd party type in the list of parameters or as a return value type, but this was not the case. Other functions with similar types seemed to convert just fine and if I add "virtual void f() = 0;" to the interface and "virtual void f();" to the implementation class, Visual Assist still does not detect the issue in this case either.
As soon as I add 'void' in the parameters list however, it does complain. So parsing is happening, but some functions are just not detected as missing an 'override'.
I apologize for the lack of predictable steps to reproduce. I could not narrow them down at this point. I would appreciate any help in understanding this issue.
My environment:
Visual Assist: 10.9.2237.0
MS Visual Studio 2015 Update 3, English US
Thank you for your help!
|
| 8 L A T E S T R E P L I E S (Newest First) |
| feline |
Posted - Oct 24 2017 : 3:46:41 PM
This looks like a workable idea, so I have put in a feature request to see what our developers make of this idea:
case=111951 |
| zzdev10 |
Posted - Oct 23 2017 : 07:40:00 AM
That would definitely be helpful. Also, if there would be a place like logs for example, were you could go to explore the whole list if there will be more than 5 unknown symbols. I suspect that as people adjust to and use code inspection the list will get shorter, but we will need to know the list of missing symbols in order to adjust the code.
|
| feline |
Posted - Oct 21 2017 : 07:18:56 AM
Thinking about this some more, since knowing that there was something Code Inspection didn't understand would be helpful, how about a "marker" of some sort, in the Code Inspection Results window, saying "24 unknown symbols were not checked", and a tooltip over the marker showing the first 5 symbols that were not checked?
It gives you symbols, most likely class names to search the file for, without trying to tell you to much. Does this sound like a sensible middle ground?
This is just me thinking out loud, I don't know if Code Inspection collects this type of information, but I would like to see some way of reporting what could not be checked. It would make supporting Code Inspection easier  |
| zzdev10 |
Posted - Oct 20 2017 : 9:20:24 PM
Thank you so much for the detailed explanation. I now understand the difficulty behind implementing of such request.
I look forward to upcoming releases to see the fix for the header include issue, as well as new code inspection checks. I really like this feature! |
| feline |
Posted - Oct 20 2017 : 6:04:59 PM
Thank you for your checks, and the results. The clue for me is that this works as expected when setting an absolute include directory, but not a relative include directory. This looks like it is:
case=111596
which is to do with how code inspection checks for header files that are included in header files. This has been fixed internally, and hopefully the fix will be in the next build of VA.
The difference between "I could not run the check" and "found nothing" is interesting. I am just not sure how to report this sensibly. Consider this example, a header file contains the following code:
typedef int testTypedefOne;
typedef std::vector<int> testTypedefTwo;
but the header file does NOT include the #include for std::vector. So long as the cpp file that includes this header file has the include, all is good.
Code Inspection checks the header file in isolation, and in isolation the second typedef line cannot be understood. So only the first typedef line is marked to indicate that the typedef can be converted to a using declaration.
So, we cannot say that the convert typedef to using check is never run, since it is run, and it found an issue. Do we report that there were X lines of code that were ignored? If so, you are going to want to know which lines, but this could be large parts of the file.
Reporting that there was something code inspection could not resolve, so could not check, makes sense. But saying this without providing some information about what could not be resolved seems to be the worst of both worlds. |
| zzdev10 |
Posted - Oct 20 2017 : 2:43:22 PM
As I was trying to narrow down the conditions for this issue, I went back to the initial version of the implementation class header. (I have since submitted the override fixes and needed the old version to test a hypothesis that I had). Now Visual Assist had found all of the cases, but the file-specific issue came back regardless of the project settings. I had to specify full explicit path in the header in order for VA to pick up the interface declaration header. This explains why my later attempts to add anything to the interface declaration file did not result in new warnings. Even though I do not know why it partially worked on the same class the first time, I'd like to withdraw this issue until I see another problem.
As feline mentioned before, I do not think that there is a within-class problem, but rather a problem of locating the header file.
Based on this, I have 2 questions:
1. What is the best way for VA to know where to look for other includes? We currently specify it in the project file as $(MainProjectDir)\include, while the rest of the project files are located in $(MainProjectDir)\SubDir\SubProject
2. Is there any way I can find out if VA was not able to find a header file, included in the current file? It would be good to differentiate between "I could not run the check" case and "no errors" case.
Thank you for your help, |
| zzdev10 |
Posted - Oct 20 2017 : 1:11:34 PM
Q: Is it class-specific?
A: No. Class-specific cases I was able to fix so far by correcting a search path for included files in the project file from relative to absolute. For example, we had "..\..\include". When I corrected it to $(MainProjectFolder)\include, VA would find the missing class-specific overrides.
Q: If at least one missing override is being found in a class, are all of the expected functions being marked as missing override?
A: No. That's exactly the issue. Let me try giving you a bit more context for those cases.
DECLARE_INTERFACE_(IMyWidget, IUnknown)
{
virtual HRESULT CreateChildren() = 0;
// 19 other functions
virtual WidgetType GetWidgetType() = 0;
};
class CMyWidget : public IMyWidget
{
CMyWidget( void ); // OK: identified by VA
// other constructors and a destructor
// IUnknown methods declared as following:
STDMETHOD(QueryInterface)(THIS_ REFIID riid, LPVOID * ppvObj);
STDMETHOD_(ULONG,AddRef)(THIS);
STDMETHOD_(ULONG,Release)(THIS);
virtual WidgetType GetWidgetType(); // FAILURE: override is not suggested
virtual HRESULT CreateChildren(); // OK: override is suggested. This is the very next line
};
Thank you for your help, |
| feline |
Posted - Oct 20 2017 : 09:50:10 AM
When you say 20%, does this seem to be class or file specific? If at least one missing override is being found in a class, are all of the expected functions being marked as missing override?
If this was file specific then it might make a bit more sense. It still wont tell us why it is happening in these files, but it will narrow things down a bit. |
|
|
