Clang CC

[l_inc]

yvesdm3000

As long as it doesn't give me trouble I keep it as-is, it does help to keep the lifetime of the clang TU correct without additional pointers.

I think it's rather about avoiding additional copying of the TranslationUnit data. I'm not much into C++, and this was something new to me. It's just important to remember to copy-construct objects from temporary objects only. This includes initialization and assignment as well, because the assignment constructor of TranslationUnit gets the assigned object by value and hence will corrupt it during copy-construction.

Thing is... the compiler should do copy elision for temporary objects (which includes the m_Files vector) anyway, so it should be the same speed when doing the standard colon-initialization, but without introducing hazardous conditions for normal objects. So I would really want to know the opinion of the author of this Achtung.

[yvesdm3000]

yvesdm3000
I think it's rather about avoiding additional copying of the TranslationUnit data. I'm not much into C++, and this was something new to me. It's just important to remember to copy-construct objects from temporary objects only. This includes initialization and assignment as well, because the assignment constructor of TranslationUnit gets the assigned object by value and hence will corrupt it during copy-construction.

When it's not C++11, the elements in the vector<TranslationUnit> will always be copy-constructed, they will then always be copied again when the vector grows beyond the allocated memory. Now for the clang TU, there is only 1 call to release that memory and when you have multiple copies of that same pointer, doing a clang_releaseTranslationUnit would then be done on all copies and hence freeing the same memory multiple times. Before C++11 we would have solved this with a pointer to TranslationUnit and do memory-management ourselves, with C++11 with the move operator it becomes much easier since the lifetime can be tied to the lifetime of the container elements, and with Alpha's trick of emulating the move operator it seems to perform the same task. It works, so I don't touch it.

Yves

[Alpha]

Yes, the constructor is not intended specifically for performance (though if it helps with that, any speed is welcome), but to try to make memory management as automatic as possible: simply adding to/removing from m_TranslUnits handles the necessary operations.  (Clang TU's use a lot of RAM, so leaking one is... undesirable.)

I hope those who wrote the copy-constructor of the TranslationUnit as simulating a move-contructor for speed know what they do, cause it might become unsafe.

Casting away a const and editing it is undefined if the original object was created const.  However, TranslationUnits must be mutable to interact with clang, so this is not the case.  For insurance though, I tried to do the minimum possible operations: 3 pointer copies from the vector swap(), 1 pointer copy for the clang TU.

The self swap() of m_Files frees any over allocated memory.  Since the ctor is the only place this vector is modified, it did not make sense to continue holding on to the memory.

[Alpha]

Regarding RemoveTranslationUnit(), keep in mind that multiple editors for different files may utilize the same TU.  Also, they are expensive to (re)create, so keeping them around (as memory constraints allow) after closing an editor helps smooth the workflow for people who frequently (re)open and close files.
It was my intent to update CreateTranslationUnit() to treat m_TranslUnits as a cache, allowing it to discard TU's after a certain limit (probably using a variant of LRU).

[yvesdm3000]

Regarding RemoveTranslationUnit(), keep in mind that multiple editors for different files may utilize the same TU.

I think I commented-out that capability. Every file has its own TU, mostly for simplicity for now and I'm unsure if I would ever want to re-enable that, given that an include-file might look different with defines set... My focus right now is to have the basic functionality working: have CodeCompletion when you need it, no hangs but without any optimizations regarding resources and thus making many copies across threads.

I think it's even better to cache the TU to disk as a sort-of PCH when the associated file is closed. Symbol search in closed files would then reopen each PCH.

Yves

[l_inc]

yvesdm3000

the elements in the vector<TranslationUnit> will always be copy-constructed, they will then always be copied again when the vector grows beyond the allocated memory

Yes, and is there a guarantee that they won't be copy-constructed twice from the same object? I mean there is a notion of a sane implementation, but no guarantee, right?

Before C++11 we would have solved this with a pointer to TranslationUnit and do memory-management ourselves

Before C++11, I think, I would rather implement a simplified version of a shared_ptr . This seems much safer.

Alpha

Casting away a const and editing it is undefined if the original object was created const.

My concerns are rather related to attempts to accidentally do smth. like m_TranslUnits[1] = m_TranslUnits[0]; .

[l_inc]

yvesdm3000, Alpha

Another problem is that the plugin sometimes gets into an infinite loop inside wxExecute(command, output, errors, wxEXEC_NODISABLE); . Inside the function int wxGUIAppTraits::WaitForChild(wxExecuteData& execData) to be more precise, while GTK_EndProcessDetector is always getting 0 from waitpid and g++ is somehow not present in the process list.

OK. I was a bit wrong. The infinite loop in WaitForChild results from the call wxExecute(cmd,                 output, wxEXEC_NODISABLE);, which is in "sdk / globals.cpp" . The command line is "wx-config --version=2.8 --cflags". It happens on opening a project, but I don't know what conditions it needs. wxconfig started by the call keeps hanging in the process list, endProcData->pid in wxGUIAppTraits::WaitForChild(wxExecuteData& execData) is negative (not sure yet, if it's supposed to be negative) with an absolute value equal to the hanging process. Here's the call stack:

Code

#0 0x7fb2a8045590	wxGUIAppTraits::WaitForChild(wxExecuteData&) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#1 0x7fb2a8045bfb	wxExecute(wchar_t**, int, wxProcess*) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#2 0x7fb2a8046345	wxExecute(wxString const&, int, wxProcess*) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#3 0x7fb2a804232b	wxDoExecuteWithCapture(wxString const&, wxArrayString&, wxArrayString*, int) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#4 0x7fb2a8a00851	ExpandBackticks(wxString&) (str=...) (globals.cpp:870)
#5 0x7fb2a89a54d5	CompilerCommandGenerator::SetupCompilerOptions(Compiler*, ProjectBuildTarget*) (this=0x59b9e80, compiler=0x2e97240, target=0x59b4000) (compilercommandgenerator.cpp:937)
#6 0x7fb2a89aa3a2	CompilerCommandGenerator::Init(cbProject*) (this=this@entry=0x59b9e80, project=project@entry=0x5982ae0) (compilercommandgenerator.cpp:161)
#7 0x7fb275f511c9	CompilerMINGW::GetCommandGenerator(cbProject*) (this=<optimized out>, project=0x5982ae0) (compilerMINGW.cpp:51)
#8 0x7fb27f2e0ede	ClangPlugin::UpdateCompileCommand(cbEditor*) (this=0x23b0b30, ed=0x5a16ca0) (/tmp/l.inc/ClangLib/clangplugin.cpp:995)
#9 0x7fb27f2decaa	ClangPlugin::OnEditorActivate(CodeBlocksEvent&) (this=0x23b0b30, event=...) (/tmp/l.inc/ClangLib/clangplugin.cpp:675)
#10 0x7fb27f2f4f5c	cbEventFunctor<ClangPlugin, CodeBlocksEvent>::Call(CodeBlocksEvent&) (this=0x2bed0e0, event=...) (/tmp/l.inc/codeblocks-svn/src/include/cbfunctor.h:49)
#11 0x7fb2a8a15930	Manager::ProcessEvent(CodeBlocksEvent&) (this=<optimized out>, event=...) (manager.cpp:263)
#12 0x7fb2a8a22e35	PluginManager::NotifyPlugins(CodeBlocksEvent&) (this=<optimized out>, event=...) (pluginmanager.cpp:1450)
#13 0x7fb2a89e3ba8	EditorManager::SetActiveEditor(EditorBase*) (this=this@entry=0x1ee2d40, ed=0x5a16ca0) (editormanager.cpp:468)
#14 0x7fb2a89e52c3	EditorManager::CheckForExternallyModifiedFiles() (this=0x1ee2d40) (editormanager.cpp:972)
#15 0x7fb2a803e47c	wxEvtHandler::ProcessEventIfMatches(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#16 0x7fb2a803e533	wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#17 0x7fb2a803e8bf	wxEvtHandler::ProcessEvent(wxEvent&) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#18 0x7fb2a803e3d8	wxEvtHandler::ProcessPendingEvents() () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#19 0x7fb2a7fb5e69	wxAppConsole::ProcessPendingEvents() () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#20 0x7fb2a80f54ee	wxAppBase::ProcessIdle() () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#21 0x7fb2a805f705	wxapp_idle_callback() (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#22 0x7fb2a5c98a8a	g_main_context_dispatch() (/lib64/libglib-2.0.so.0:??)
#23 0x7fb2a5c98e20	g_main_context_iterate.isra() (/lib64/libglib-2.0.so.0:??)
#24 0x7fb2a5c98ecc	g_main_context_iteration() (/lib64/libglib-2.0.so.0:??)
#25 0x7fb2a7743151	gtk_main_iteration() (/lib64/libgtk-x11-2.0.so.0:??)
#26 0x7fb2a805fc85	wxApp::Yield(bool) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#27 0x7fb2a8045595	wxGUIAppTraits::WaitForChild(wxExecuteData&) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#28 0x7fb2a8045bfb	wxExecute(wchar_t**, int, wxProcess*) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#29 0x7fb2a8046345	wxExecute(wxString const&, int, wxProcess*) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)
#30 0x7fb2a804232b	wxDoExecuteWithCapture(wxString const&, wxArrayString&, wxArrayString*, int) () (/opt/wx/2.8/lib/libwx_gtk2u-2.8.so.0:??)

OK. This was an easy one. [...] Patch attached.

No, it wasn't. Now code completion related to the closed file is dead. I'll try to have a look, but I hope one of you guys would fix it, cause it seems I miss many assumptions needed to be considered.

[yvesdm3000]

No, it wasn't. Now code completion related to the closed file is dead. I'll try to have a look, but I hope one of you guys would fix it, cause it seems I miss many assumptions needed to be considered.

Yeah I understand code completion is probably dead once you have closed (and reopened) a file. That will certainly be fixed with the upcoming push. This push will feature a duplicate TranslationUnit for the current file that is being reparsed, allowing the typing user to use Code Completion while it's being reparsed. It will also parse a TranslationUnit when the clang TU associated with it is missing, such as is the case once you've closed and reopened a file.

Yves

[l_inc]

yvesdm3000
OK. But as of now I've just restored the commented behaviour of GetTranslationUnitId for the OnEditorClose handler only. I suppose this way the translation unit is only killed when its root file is closed, which seems to me most reasonable.

Patch attached. The previous patch should be applied before.

[yvesdm3000]

Mmh, I seem to have run into the same issue when expanding the backticks:

#0  0x00002b410eb4349b in wxPipeInputStream::CanRead() const () from /home/yves/codeblocks-1510/lib/libwx_gtk2u-2.8.so.0
#1  0x00002b410eb446f1 in wxGUIAppTraits::WaitForChild(wxExecuteData&) ()
   from /home/yves/codeblocks-1510/lib/libwx_gtk2u-2.8.so.0
#2  0x00002b410eb42e42 in wxExecute(wchar_t**, int, wxProcess*) ()
   from /home/yves/codeblocks-1510/lib/libwx_gtk2u-2.8.so.0
#3  0x00002b410eb426d7 in wxExecute(wxString const&, int, wxProcess*) ()
   from /home/yves/codeblocks-1510/lib/libwx_gtk2u-2.8.so.0
#4  0x00002b410eb4046b in wxDoExecuteWithCapture(wxString const&, wxArrayString&, wxArrayString*, int) ()
   from /home/yves/codeblocks-1510/lib/libwx_gtk2u-2.8.so.0
#5  0x00002b411ef03486 in ClangPlugin::GetCompilerInclDirs (this=0x3be3ae0, compId=...)
    at /mnt/data/ClangLib/clangplugin.cpp:856
#6  0x00002b411ef03c62 in operator= (this=0x3be3dd8) at /home/yves/wxWidgets-2.8/include/wx-2.8/wx/string.h:659
#7  ClangPlugin::UpdateCompileCommand (this=0x3be3ae0, ed=<optimized out>) at /mnt/data/ClangLib/clangplugin.cpp:1070

I fixed a similar issue before where the UpdateCompileCommand was run inside a timer, and wxExecute also ran timers which in turn ran again UpdateCompileCommand with a wxExecute...   But in this case, it's not a timer issue... Looks more like a WX issue.

Yves

[yvesdm3000]

Regarding RemoveTranslationUnit(), keep in mind that multiple editors for different files may utilize the same TU. 

Coming back on the problem of header files (and in theory other .CPP files, although very very bad practice) linked to the same TU. The problem is pretty complex... If a header-file is self-contained, it can have it's own TU, but --- although this is a good coding practice --- we cannot assume this will always be the case. Even more, a header-file might be freshly created, not referenced in any other file so it gets his own TU and then included in another .CPP file. In this case, we should get rid of the TU since it will also be parsed as part of the CPP file.

And then we have the case of different #defines  set before the inclusion of a header file, which version do we show ? I'm thinking of having a way of preferring the TU that is currently open and prefer that TU so whenever you go to the .CPP file and you select the header file there to open it, it would start using the context of that TU. The only issue here is a desync of how Scintilla will grey out #defines and how CC operates.

For removal, in the short term I'm releasing the TU when the 'main' file that requested the creation of the TU is closed. It can always be reloaded once it's needed (upcoming functionality) and I think I should also start looking into building .PCH files. When we have PCH files from TU's, reloading them is less painful.

Yves

[l_inc]

yvesdm3000

For removal, in the short term I'm releasing the TU when the 'main' file that requested the creation of the TU is closed.

This is pretty much exactly what my little patch does.

[yvesdm3000]

I created a branch "devel" which does not necessarily work correctly, but it should show people what is being fixed and what might allready be fixed.
The current state of that devel-branch is that the main TU is working correctly, but the alternate TU is only giving 0 CodeCompletion results back (no error!). Since each reparse swaps TU, you only get code completion on one TU, and after a reparse you get 0...

I'll have to step thru clang src to see why it behaves like that.

Regarding caching PCH files, is there a good Code::Blocks location where I should place these? Some infrastructure, or is wxStandardPaths::GetTempDir good enough ?

Yves

[l_inc]

yvesdm3000

Regarding caching PCH files, is there a good Code::Blocks location where I should place these? Some infrastructure, or is wxStandardPaths::GetTempDir good enough ?

Do you really mean precompiled headers, or is it supposed to be a code parser database (like .sdf in Visual Studio)? As for the latter I'd prefer to keep the corresponding information in memory. And you can make an option to store it periodically on disk at a user-defined location. GetTempDir is a good default for such a location.

Btw. how is it going with integrating the plugin into codeblocks-contrib?

[yvesdm3000]

yvesdm3000

Regarding caching PCH files, is there a good Code::Blocks location where I should place these? Some infrastructure, or is wxStandardPaths::GetTempDir good enough ?

Do you really mean precompiled headers, or is it supposed to be a code parser database (like .sdf in Visual Studio)? As for the latter I'd prefer to keep the corresponding information in memory. And you can make an option to store it periodically on disk at a user-defined location. GetTempDir is a good default for such a location.

Btw. how is it going with integrating the plugin into codeblocks-contrib?

For PCH, it seems like I can save a whole Translation Unit into a PCH file. Not really useful for code-completion, but for operations like 'go to implementation', 'go to declaration', 'show references' to work efficiently, it's better to have ALL files parsed into AST trees. Obviously we can't assume users will always have enough memory to store that in memory, especially for pretty large projects. So for example 'goto implementation', it would first look in any TU in memory, if it can't find it, it would go into a slower mode and open each PCH file one by one and see if it can find what it's looking for.

For codeblocks-contrib, not sure if I am going to do this immediately, I think the plugin needs restructuring first. I want to split the plugin into pieces:

1. ClangLib, just updating its internal database, parsing documents as they are loaded&changed and governing the threads for doing the tasks it publishes in its public API
2. ClangCC using ClangLib API, it will perform CodeCompletion and associated settings nothing more
3. ClangToolbar using ClangLib API, it will allow a toolbar with class & method name
4. ClangCodeRefactor using ClangLib API, it will allow context-menu's to lookup declaration, implementation and maybe higher level code refactoring stuff like reference searching and symbol renaming

Yves