Bug 336810 - Incorrect namespace assignment of base class with c++ import
Summary: Incorrect namespace assignment of base class with c++ import
Status: RESOLVED FIXED
Alias: None
Product: umbrello
Classification: Applications
Component: general (show other bugs)
Version: 2.13.2
Platform: unspecified Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Umbrello Development Group
URL:
Keywords:
: 193080 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-06-27 20:44 UTC by Ralf Habacker
Modified: 2019-07-12 22:04 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 2.13.80 (KDE 4.13.80)


Attachments
c++ header resulting into the wrong base class assignment (108 bytes, text/x-chdr)
2014-06-27 20:46 UTC, Ralf Habacker
Details
Simplified namespace test case mentioned in comment 5 (484 bytes, text/x-chdr)
2014-07-01 12:51 UTC, Ralf Habacker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2014-06-27 20:44:41 UTC
Importing the following code fragment 

namespace NameSpace {
class NameSpacedClass {};
class AnotherNameSpacedClass : public NameSpacedClass {};
}

results into wrong namespace assignments of the base class. 


Reproducible: Always

Steps to Reproduce:
1. open umbrello
2. import the appended c++ header

Actual Results:  
The class NameSpacedClass() is located in the global name space, which is wrong. 

Expected Results:  
The class NameSpaceClass() should be located into the NameSpace namespace
Comment 1 Ralf Habacker 2014-06-27 20:46:30 UTC
Created attachment 87437 [details]
c++ header resulting into the wrong base class assignment
Comment 2 Ralf Habacker 2014-06-27 20:59:28 UTC
The problem is that in Import_Utils::createGeneralization() 

void createGeneralization(UMLClassifier *child, const QString &parentName)
{
    UMLObject *parentObj = createUMLObject(UMLObject::ot_Class, parentName);
    UMLClassifier *parent = static_cast<UMLClassifier*>(parentObj);
    createGeneralization(child, parent);
}

createUMLObject() is called with the global namespace as parent (third parameter=0), which search (and creates it not found) the related class in the global namespace, while it is required to find the namespace of the parent class first and to provide it to createUMLObject().
Comment 3 Oliver Kellogg 2014-06-28 08:23:49 UTC
Git commit 0160b7d8f8f76f3c2da00012314f0e4454c7105e by Oliver Kellogg.
Committed on 28/06/2014 at 08:23.
Pushed by okellogg into branch 'master'.

umbrello/codeimport/kdevcppparser/cpptree2uml.cpp
- In function parseBaseClause() first call Import_Utils::createUMLObject()
  for creating the parent object coresponding to baseName, and then call
    Import_Utils::createGeneralization(UMLClassifier*, UMLClassifier*)
  with the parent object obtained.

umbrello/codeimport/import_utils.cpp
- At function
    createGeneralization(UMLClassifier *child, const QString &parentName)
  mention its shortcoming and mention the preferred solution as applied
  above.

M  +4    -0    umbrello/codeimport/import_utils.cpp
M  +5    -1    umbrello/codeimport/kdevcppparser/cpptree2uml.cpp

http://commits.kde.org/umbrello/0160b7d8f8f76f3c2da00012314f0e4454c7105e
Comment 4 Ralf Habacker 2014-06-28 09:50:54 UTC
(In reply to comment #3)
> Git commit 0160b7d8f8f76f3c2da00012314f0e4454c7105e by Oliver Kellogg.
> Committed on 28/06/2014 at 08:23.
> Pushed by okellogg into branch 'master'.
> 
> umbrello/codeimport/kdevcppparser/cpptree2uml.cpp
> - In function parseBaseClause() first call Import_Utils::createUMLObject()
>   for creating the parent object coresponding to baseName, and then call
>     Import_Utils::createGeneralization(UMLClassifier*, UMLClassifier*)
>   with the parent object obtained.
This fixes the mentioned case, thanks. Unfortunally the fix seems not to be complete because with the following testcase class GlobalA is shown in Logical View but assigned to namespace NameSpace which is wrong.

class GlobalA {
public:  
  GlobalA();
};

namespace NameSpace {

class NameSpacedC : public GlobalA
{
public:
  NameSpacedC();
};
}

The following testcase fails too
/// class Global::A
class A { public:  A(); };

namespace N1 {

  /// class N1::A
class A { public:  A(); };
}

namespace N2 {

/// class N2::A
class A { public:  A(); };

/// class N2::B
class B : public A { public: B(); };

/// class N2::C
class C : public ::A { public; C(); };

/// class N2::D
class C : public N1::A { public; C(); };
}
Comment 5 Ralf Habacker 2014-06-28 09:59:15 UTC
An additional failing testcase

/// class Global::A
class A { public:  A(); };

namespace N1 {

  /// class N1::A
class A { public:  A(); };
}

namespace N2 {

  /// class N2::A
class A { public:  A(); };
}

using N2;

namespace N3 {

/// class N3::A
class A { public:  A(); };

/// class N3::B
class B : public A { public: B(); };

/// class N3::C
class C : public ::A { public; C(); };

/// class N3::D
class C : public N1::A { public; C(); };

/// class N3::E
class D : public N2::A { public; C(); };
}
Comment 6 Oliver Kellogg 2014-06-28 18:38:45 UTC
Git commit f90f771bc4ee756d7e9e3e53794d889ef802e45f by Oliver Kellogg.
Committed on 28/06/2014 at 18:38.
Pushed by okellogg into branch 'master'.

(In reply to comment #4)
> [...] Unfortunately the fix seems not to be complete because with
> the following testcase class GlobalA is shown in Logical View but
> assigned to namespace NameSpace which is wrong.

Here is the fix for this case.
The fixing of your other test cases is still wide open, help appreciated.

umbrello/codeimport/kdevcppparser/cpptree2uml.cpp
- In function parseBaseClause(), before calling
  Import_Utils::createUMLObject() for creating the parent object, call
  Import_Utils::putAtGlobalScope(true). After creation of parent object,
  call Import_Utils::putAtGlobalScope(false).

M  +2    -0    umbrello/codeimport/kdevcppparser/cpptree2uml.cpp

http://commits.kde.org/umbrello/f90f771bc4ee756d7e9e3e53794d889ef802e45f
Comment 7 Ralf Habacker 2014-06-30 06:26:18 UTC
(In reply to comment #6)
> Git commit f90f771bc4ee756d7e9e3e53794d889ef802e45f by Oliver Kellogg.
> Committed on 28/06/2014 at 18:38.
> Pushed by okellogg into branch 'master'.
> 
> (In reply to comment #4)
> > [...] Unfortunately the fix seems not to be complete because with
> > the following testcase class GlobalA is shown in Logical View but
> > assigned to namespace NameSpace which is wrong.
> 
> Here is the fix for this case.
works, thanks 

> The fixing of your other test cases is still wide open, help appreciated.

Because createUMLObject already has a basic object find strategy inside I assume that we need to split/refactor createUMLObject to get better control where and what to search ?
Comment 8 Ralf Habacker 2014-07-01 10:28:40 UTC
(In reply to comment #5)
> An additional failing testcase
> 
...

> namespace N1 {
> 
>   /// class N1::A
> class A { public:  A(); };
> }
> 
The problem here is that N1::A shows package path N1 in properties (which is correct), but is shown directly below logical view in the tree view and also could not moved into N1 namespace
Comment 9 Ralf Habacker 2014-07-01 10:53:53 UTC
(In reply to comment #8)
> (In reply to comment #5)
> > An additional failing testcase
> > 
> ...
> 
> > namespace N1 {
> > 
> >   /// class N1::A
> > class A { public:  A(); };
> > }
> > 
> The problem here is that N1::A shows package path N1 in properties (which is
> correct), but is shown directly below logical view in the tree view
In detail: On calling import_utils.cpp::CreateUmlObject() to create N1::A  the call to 
UMLObject * o = umldoc->findUMLObject(name, type, parentPkg);

on line import_utils.cpp:209 returns the UMLObject of class A  in the global namespace which is wrong; it should return zero. 

The reason why the global object is returned is documented in Model_Utils::findUMLObject()

 * @param currentObj    Object relative to which to search (optional.)
 *                      If given then the enclosing scope(s) of this
 *                      object are searched before the global scope.

The search should be limit to the namespace given by currentObj.
Comment 10 Ralf Habacker 2014-07-01 12:51:50 UTC
Created attachment 87497 [details]
Simplified namespace test case mentioned in comment 5

Remaining tree view issue is that class N1:A is not located in the N1 package.
Comment 11 Ralf Habacker 2014-07-01 14:36:02 UTC
Git commit b37eebda3b4b0387486fe80d0e329a73c8f6de72 by Ralf Habacker.
Committed on 01/07/2014 at 14:08.
Pushed by habacker into branch 'master'.

Fix bug not placing class N1::A below N1 package in tree view.

The problem was, that Import_Utils::createUMLObject() tries to find an object
in different namespaces and only creates an uml object if not found.

In the case of parsing a c++ import class specification we simply need
to create the related uml class object in the current namespace, which
is provided by the additional parameter "doNotSearch".

Related test case is https://bugs.kde.org/show_bug.cgi?id=336810#c10

M  +11   -3    umbrello/codeimport/import_utils.cpp
M  +2    -1    umbrello/codeimport/import_utils.h
M  +1    -1    umbrello/codeimport/kdevcppparser/cpptree2uml.cpp
M  +4    -0    umbrello/object_factory.h

http://commits.kde.org/umbrello/b37eebda3b4b0387486fe80d0e329a73c8f6de72
Comment 12 Ralf Habacker 2014-07-01 20:09:43 UTC
Git commit 76fa95164959012384fca4c524dcbba548abc496 by Ralf Habacker.
Committed on 01/07/2014 at 20:07.
Pushed by habacker into branch 'master'.

Move creating of uml artifact objects into separate method Import_Utils::createArtifact().

M  +26   -15   umbrello/codeimport/import_utils.cpp
M  +4    -0    umbrello/codeimport/import_utils.h
M  +1    -1    umbrello/codeimport/kdevcppparser/cpptree2uml.cpp

http://commits.kde.org/umbrello/76fa95164959012384fca4c524dcbba548abc496
Comment 13 Ralf Habacker 2014-07-01 21:30:36 UTC
*** Bug 193080 has been marked as a duplicate of this bug. ***