Community
Participate
Working Groups
Example: void foo(int bar1, const int & bar2) { } int main() { foo(1, test); return 0; } Apply the Create local variable quick fix to 'test'. It will create 'const int & test;'... this will not compile, it needs a reference and I think most of the times the user will want it to non-const. So I think it should just be 'int test;'
Created attachment 197986 [details] fix This patch seems to fix the issue as far as type inference from function calls is involved. I've left the inference from operators (op= specifically) as it is - simply use the other hand side's type as it is - since in that case I believe we cannot reliably guess what the user meant.
I've noticed that this code can use some rewriting, which should improve both the readability and the performance. Marking obsolete & going to provide another patch
Created attachment 198102 [details] fix 2 Updated version with rewritten code and relevant extensions in DeclarationGenerator
Is there tests in the patch? I cannot find them
Created attachment 198966 [details] test cases
Thanks Tomasz, I will look at the patch and tests. I glanced at the code and saw that there is some new API, it's a bit unfortunate that a fix like this can't go into 8.0.x. Could we back port the current patch to 8.0? I assume it would be a mixture of the current patch and the previous one. I guess I'll figure this out when I look at the code more closely.
I assume the problem is that the API is extended (even though its a non breaking change), but it still would need to go to 8.1 not for instance 8.0.1 because of that?
(In reply to comment #7) > I assume the problem is that the API is extended (even though its a non > breaking change), but it still would need to go to 8.1 not for instance 8.0.1 > because of that? Yes. AFAIK, making a non-breaking API change means changing the minor segment, it is a "externally visible" change.
Created attachment 199258 [details] fix 2 for 8.0 DeclarationGenerator needs changes for this patch. So if we don't want to extend the API, the best solution I see is to create a duplicate of DeclarationGeneratorImpl. The duplicate would be internal to Codan only for the time being (8.0) and contain new functionality (from 8.1). 8.0 branch would use this duplicate (= this patch), while 8.1 branch would use the new API and no longer contain any duplicate (= previous patch). Is this a correct approach or should we solve this otherwise?
CxxAstUtils: - resolveBinding is called twice without a lock on the index Tests: - I don't think we need this line twice : assertContainedIn("int a[5]", result); - // const unsigned char* const c[3]; // cptr = c; First line, let's assign 'c' to something for the sake of it compiling at the end. This produces: const unsigned char **const cptr; but shouldn't it be: const unsigned char * const * cptr; ? Not sure what we'll do about 8.0 yet but at least creating a duplicate would work for now. I'm just worried that there might be another fix that would also benefit other clients, like refactoring, then we'd have to copy it there too.
> - resolveBinding is called twice without a lock on the index Hm... Shouldn't it? I thought the lock is just for operations on an IIndex. And anyway- doesn't Codan lock the index for the whole checker or quick fix invocation? I'll review the rest, thanks.. I feel kind of bad with that idea of code duplication, it just feels wrong to have a different patch for 8.0 and 8.1, sounds like it's going to make problems in the future (and by problems I mean preparing half of next fixes in 2 different versions). I'll try to think of something better... When is 8.1 going to come out?
Created attachment 200055 [details] resolveBinding using Index (In reply to comment #11) > > - resolveBinding is called twice without a lock on the index > > Hm... Shouldn't it? I thought the lock is just for operations on an IIndex. And > anyway- doesn't Codan lock the index for the whole checker or quick fix > invocation? resolveBinding can indirectly use the index when name.getTranslationUnit().getIndex() is not null, see attachment for example. You are right that in the case of Codan, the index is locked but the method createDeclaration is public API (should it really be?); it can be called from anywhere so a client could call it without having a lock on the index. Just to make sure, I tried creating a plug-in and I could call it without any warning. > When is 8.1 going to come out? 8.0.1 is September 2011, 8.1/9.0/next is June 2012 (Juno).
Created attachment 200445 [details] updated fix + tests (merged 347523) I tackled the problem from a different angle. Instead of extending DeclarationGenerator, I moved the "type postprocessing" logic to the type inference functions in CxxAstUtils. I have a feeling that it belongs there more - it's really a matter of context what exactly should be done with the type. DeclarationGenerator can't and shouldn't be concerned with this. This also magically fixed the issue with array degradation you've mentioned. DeclarationGenerator now doesn't change the given type at all (it used to degrade functions and arrays to pointers). Should've been like this from the start- just generate a declaration for whatever type it gets. Anyway, I'll need to have a look at the refactorings later, since they also use it in some places (and deserve a similar approach). Also: I needed to merge in the fix (and test) from 347523 since the two were interleaved and it was significantly easier to clean this up as a whole.
Created attachment 202293 [details] modified fix + tests (In reply to comment #13) > DeclarationGenerator now doesn't change the given type at all (it used to > degrade functions and arrays to pointers). Should've been like this from the > start- just generate a declaration for whatever type it gets. If that's the case then I think we should remove the "dead" code in DeclarationGenerator, i.e. the convertArrayToPointer and convertFunctionToFuncPtr stuff. In CxxAstUtils, because IPointerType is subclassed, I get a API warning: "CxxAstUtils illegally implements IPointerType". In my patch, I instantiated CPointer and CPPPointer instead. Not sure what is worse, but the warnings from discouraged access can be suppressed more cleanly. It would be nice to have a better solution. Any ideas? > Anyway, I'll need > to have a look at the refactorings later, since they also use it in some places > (and deserve a similar approach). Some tests fail in ExtractConstant. So this needs to be fixed simultaneously if we don't want regressions. I added DeclarationGenerationHelper with a copy-pasted pointerTo method so that the other refactorings could use this. I feel like we should have a better way to share code between codan and refactoring. I'm not sure putting a lot of utils method in core is the right solution. DeclarationGeneratorImpl.createDeclSpecFromTypeImpl and createDeclaratorFromTypeImpl : I don't think they need to be public, but they're not in my patch anyway. They were not needed since ignoreInitialCVQualifier == false
Tomasz, do you plan on addressing the remaining comments and getting this patch committed?
