#1061 Fix Symbol Browser compilation when CC_NO_COLLAPSE_ITEM is undefined

[Miguel Gimenez]

Currently compilation of classbrowser.cpp fails when CC_NO_COLLAPSE_ITEM is not defined. This macro is defined in CBP files, but not when using autotools, so the fail was not detected when it should. The attached patch removes place holder GetId() call and adds the real code.

I think CC_NO_COLLAPSE_ITEM should be defined (or not) in all builds, current status is error prone and complicates debugging because the program behaves differently depending on how it was build.

[Teodor Petrov]

I think CC_NO_COLLAPSE_ITEM should be defined (or not) in all builds

Yes, it should, but I don't see you doing it in this patch.

[Miguel Gimenez]

First, I don't know autotools. Second, before changing anything it must be decided if macro definition must be removed from the C::B projects or added to the makefile (or sources modified to ignore it altogether).

IMHO, it is better to define the macro everywhere now and change the code after some months of mileage to ignore the macro .

[Lieven de Cock]

what's the status, since the build is still broken due to this ?

[Miguel Gimenez]

The patch fixes compilation with autotools, a later patch should add CC_NO_COLLAPSE_ITEM to autotools or remove it from the CBP files. This is an optimization flag and its effect is minimal.

I don't know if Morten MacFly knows this has been assigned to him.

[Lieven de Cock]

I will apply this patch, but the CC_NO_COLLAPSE_ITEM subject remains open.

[Lieven de Cock]

applied
do we keep this open for the follow up discussion on CC_NO_COLLAPSE_ITEM ?

[Teodor Petrov]

Yes, keep it open. Someone should step up and do the work.
If I understand it correctly currently we'll ship different builds on linux and windows.

[Miguel Gimenez]

The attached patch removes checking of CC_NO_COLLAPSE_ITEM from code, leaving the code as if it were defined, and also removes the macro definition from the CBP files.

This does not change anything when using CBP files, but makes linux build with autotools identical to the build with CBP.

[Teodor Petrov]

Is this last patch still needed? Anything else to do here?

[Miguel Gimenez]

Currently Linux and MSW generated binary differ slightly due to the absence of CC_NO_COLLAPSE_ITEM in Linux makefiles. This has little sense, and the most reliable way to fix this is removing support for this macro instead of messing with autotools.

[Teodor Petrov]

I don't get this - why have you removed all the code and not just ifdef and make this code always enabled?

[Miguel Gimenez]

I want the code always disabled (it is inside #ifndef).

The code is a very minor optimization with major code obfuscation. I removed it because it will be hardly needed and I don't want/know how to mess with autotools to define CC_NO_COLLAPSE_ITEM

[ollydbg]

Hi, I see this patch is in our trunk now. (in r12314)

I think the code wrapped by CC_NO_COLLAPSE_ITEM #if condition is for optimization.

When a node is expand, the child nodes (sub tree) will get calculated.
When a node is collapsed, the child nodes will get deleted.

I don't think this is wrong here, do you? Because the symbol tree is big, and some node(scope) could have many child nodes, so keep the tree dynamic is good from my point of view.

So, generally I think those code should be enabled by default.

[Miguel Gimenez]

That code was already disabled (in MSW) long before the symbol browser was proscribed, only Linux used it, but I think it is because the author forgot about it. The code completion trees usually have shallow branches (they have few leaves), so keeping the branch when collapsing it is not a problem.

The patch could be reverted, but then either CBP files or autotools files must be modified so both define CC_NO_COLLAPSE_ITEM or both have it undefined.

[Teodor Petrov]

@ollydbg If you think and can show a use case where this code is need feel free to restore it. For now I'm closing this ticket.