Style-off

[Sheldor]

AP0LL0 schrieb:

Nein, aber muss ja auch keiner. Ich finde es immer sehr irritierend wenn Leute sich wirklich über meinen Code "beschweren" und wollen dass ich ihn ändere.

Solange dir der Style gefällt und du den Code gut lesen kannst musst du ihn ja nicht ändern. (Wäre aber sehr empfehlenswert)
Falls sich die Mitarbeiter beschwerden musst du ihn ändern.

Wenn du alleine Arbeitest ist es aber nicht Problematisch, erst wenn das Produkt von jemand anderem weiterentwicklet wird.

Ich hätte z.B nicht wirklich Lust mit dem Quelltext arbeiten zu müssen.

[hustbaer]

Ist aus einem aktuellen Projekt, unbearbeitet, bis auf
* Projektname Copyright und Original Author geändert
* bei "[ snip ]" hab ich jeweils Teile rausgeschnitten (ich kann nicht gut das ganze File hier posten, wäre auch ziemlich gross)

Wobei mir gerade auffällt dass das erste Kommentar in Control::PrivateAddChild Mist ist ("do nothing" != "throw exception").

/////////////////////////////////////////////////////////////////////////////
// Copyright: Blubb
// Original Author: Mein Name
/////////////////////////////////////////////////////////////////////////////

#include "PrecompiledHeader.h"
#include <Snootch/Gui/Control.h>
#include <Snootch/Gui/ChildControlCollection.h>
#include <Snootch/Gui/LayoutManager.h>
#include <Snootch/Gfx/GfxUtility.h>

/////////////////////////////////////////////////////////////////////////////

namespace Snootch { SNOOTCH_DONT_INDENT_NAMESPACE
namespace Gui {

	/////////////////////////////////////////////////////////////////////////////

// [ snip ]

	void Control::SetLayoutManager(shared_ptr<LayoutManager> const& layoutManager)
	{
		if (layoutManager != m_layoutManager)
		{
			shared_ptr<EventConnection> dataChangedEventConnection;

			if (layoutManager)
			{
				if (GetLifecyclePhase() == LifecyclePhase_FullyInitialized)
				{
					shared_ptr<Event> const dataChangedEvent = layoutManager->GetDataChangedEvent();
					if (dataChangedEvent)
					{
						dataChangedEventConnection = dataChangedEvent->ConnectWeak(
							shared_from_this(), &Control::PrivateOnLayoutManagerDataChangedEvent);
					}
				}
			}

			SetLayoutDirty();

			m_layoutManager = layoutManager;
			m_layoutManagerDataChangedEventConnection = dataChangedEventConnection;
		}
	}

	void Control::PrepareChildren(Gfx::RenderLock& renderLock)
	{
		// Prepare in reverse Z order (=render order)
		BOOST_REVERSE_FOREACH(shared_ptr<Control> const& child, GetChildren().GetVisibleOrdered())
			child->PrepareImpl(renderLock);
	}

	void Control::RenderChildren(Gfx::RenderLock& renderLock, Vector2F const& offset, GFloat alpha)
	{
		// Render in reverse Z order (=overdraw)
		BOOST_REVERSE_FOREACH(shared_ptr<Control> const& child, GetChildren().GetVisibleOrdered())
			child->RenderImpl(renderLock, offset, alpha);
	}

	Control* Control::HitTestChildren(Vector2F const& relativePosition)
	{
		// Loop through child controls, topmost first.
		// Stop on the first child controls that wanted the click.

		// Hit test in forward Z order
		BOOST_FOREACH(shared_ptr<Control> const& child, GetChildren().GetVisibleOrdered())
		{
			Control* const hit = child->HitTestImpl(child->ParentSpaceToControlSpace(relativePosition));
			if (hit)
				return hit;
		}

		return 0;
	}

// [ snip ]

	void Control::PrivateUpdateParentCollection()
	{
		if (m_parentControl)
			m_parentControl->GetChildren().InternalUpdate(this);
	}

	void Control::PrivateSetParentCanvas(Control* parentCanvas)
	{
		SNOOTCH_ASSERT(parentCanvas == 0 || parentCanvas->m_isCanvas);

		if (m_parentCanvas != parentCanvas)
		{
			m_parentCanvas = parentCanvas;

			if (!m_isCanvas)
			{
				BOOST_FOREACH(shared_ptr<Control> const& child, GetChildren())
					child->PrivateSetParentCanvas(parentCanvas);
			}
		}
		else
		{
#ifdef _DEBUG
			PrivateCheckParentCanvasHierarchy(parentCanvas);
#endif
		}
	}

	void Control::PrivateCheckParentCanvasHierarchy(Control const* parentCanvas) const
	{
#ifdef _DEBUG
		SNOOTCH_ASSERT(parentCanvas == 0 || parentCanvas->m_isCanvas);
		SNOOTCH_ASSERT(m_parentCanvas == parentCanvas);

		if (m_isCanvas)
			parentCanvas = this;

		BOOST_FOREACH(shared_ptr<Control> const& child, GetChildren())
			child->PrivateCheckParentCanvasHierarchy(parentCanvas);
#endif
	}

	void Control::PrivateAddChild(shared_ptr<Control> const& child)
	{
		SNOOTCH_ASSERT(!child->m_parentControl);
		SNOOTCH_ASSERT(!child->m_parentCanvas);

		// If the control already has a parent, do nothing

		if (child->m_parentControl)
			throw std::invalid_argument(__FUNCTION__ ": Cannot add control as child that already has a parent.");

		// Detect loops

		for (Control* parent = this; parent != 0; parent = parent->m_parentControl)
		{
			if (parent == child.get())
			{
				SNOOTCH_ASSERT(0 && "Cannot add control as child: cannot build loops.");
				throw std::invalid_argument(__FUNCTION__ ": Cannot add control as child: cannot build loops.");
			}
		}

		// Remove capture from child control (and it's children)

		if (child->HasCapture())
			child->PrivateReleaseCapture(true);
		if (child->m_pathToCapture)
			child->PrivateStealChildCapture();

		// Insert and update parent control pointer

		m_children->InternalAdd(child);
		child->m_parentControl = this;

		// Update parent canvas pointer

		if (m_isCanvas)
			child->PrivateSetParentCanvas(this); // This control IS a canvas
		else if (m_parentCanvas)
			child->PrivateSetParentCanvas(m_parentCanvas); // This control has a parent canvas

		// Set parent canvas of the CHILD control and this control's layout dirty
		// (The parent canvas of the child control can be this control or this control's parent canvas)

		child->SetParentCanvasDirty();

		SetLayoutDirty();
	}

	void Control::PrivateRemoveChild(shared_ptr<Control> const& child)
	{
		SNOOTCH_ASSERT(child->m_parentControl == this);
		if (child->m_parentControl != this)
			return;

		// Remove capture if this child control (or one of it's children) has it

		if (child->HasCapture() || child->m_pathToCapture)
		{
			SNOOTCH_ASSERT(m_pathToCapture == child.get());

			if (child->HasCapture())
				child->PrivateReleaseCapture(true);
			if (child->m_pathToCapture)
				child->PrivateStealChildCapture();

			SNOOTCH_ASSERT(!m_pathToCapture);
		}

		// Mark parent canvas dirty so it gets redrawn

		child->SetParentCanvasDirty();

		// Clear parent control and parent canvas pointers

		child->PrivateSetParentCanvas(0);
		child->m_parentControl = 0;

		// Finally remove from collection

		m_children->InternalRemove(child);

		SetLayoutDirty();
	}

// [ snip ]

	/////////////////////////////////////////////////////////////////////////////

} // namespace Gui
} // namespace Snootch

[!rr!rr__]

wieso macht ihr alle

void bla(foo)
{
    last;
}

und nicht

void bla(foo){
    last; }

- welchem optischen Zweck dienen die leeren {- und }-Zeilen?

[LordJaxom]

!rr!rr__ schrieb:

last; }

Ih.

[Sheldor]

!rr!rr__ schrieb:

wieso macht ihr alle

void bla(foo)
{
    last;
}

und nicht

void bla(foo){
    last; }

- welchem optischen Zweck dienen die leeren {- und }-Zeilen?

Ich finde so kann man den Code besser lesen.

Es gibt noch eine Möglichkeit:

int blabla(){
return 1337;
}

[wxSkip]

@volkard: Ich weiß nicht, was du meinst, aber erzähl mal, ich sag dir dann meine Meinung dazu .

[Eisflamme]

{ sollte imo auf gleicher Höhe wie } sein. So erkennt man sofort den Block und muss nicht erst umständlich nach dem passenden { zum } suchen oder umgekehrt.

Es sieht einfach deutlich symmetrischer aus und das ist hier auch einfach deutlich schöner.

In Java macht man das wahrscheinlich nicht, weil man da ja Verschachtelungen von Klassen in ctors mit news anbietet, dass man für einen Aufruf sonst 30 Zeilen bräuchte.

Fast noch schöner als die ekligen:

void bla(){
blub;}

// oder
void bla(){
blub;
}

-Konstrukte finde ich:

void bla(){
blub;     }

Da sieht man wenigstens, dass die zusammen gehören. Ne Zeile zu sparen ist da total unterkompensierend.

Just4fun:

void bla(){
    while(true){
        blub();}
          }

usw. ^^

[volkard]

wxSkip schrieb:

@volkard: Ich weiß nicht, was du meinst, aber erzähl mal, ich sag dir dann meine Meinung dazu .

Der große Block in der Mitte erschlägt einen fast. Und macht mich sofort neugierig, weshalb es nicht ein asm bsr oder __builtin_clz ist.

[Verkehrte Welt]

Mein Code auf der Arbeit ist eine einzige Katastrophe.
Ich muss immer schnell schnell ans Ziel kommen und genauso sieht es hinterher aus.
Keine Sau interessiert sich dafür wie ich programmiere. Keine Code-Reviews, keine Absegnung des Datenbankschemas, nix. Nur das Klickibunti-Ergebnis zählt.
Wenn ich zwischendurch mal versuche mehr Wert auf guten Stil zu legen, komme ich nur zäh vorwärts. Wenn ich dagegen hacke wie ein Schwein geht alles fix, läuft stabil und jeder ist zufrieden. Vorerst zumindest. Änderungen sind entsprechend schwieriger aber andererseits habe ich genug Zeit bei der hingerotzten Erstimplementierung gespart.

Nein, ich bin damit nicht glücklich.

Privat bin ich zehnmal langsamer und gebe sehr mir viel Mühe was reine Ästhetik als auch "Architektur", Objektdesign, Entwurfsmuster, Wiederverwendbarkeit und Anwendung guter OO-Prinzipien betrifft.

[wxSkip]

@Eisflamme: Du übersiehst da ein Problem: Wenn du die Zeile mit der schließenden Klammer änderst, musst du hinten wieder Leerzeichen hinzufügen/löschen und falls sie länger wird als die vorherige, musst du auch noch Leerzeichen in der vorigen Zeile hinzufügen.

Ich habe auch mal Code in dieser Art gesehen:

void foo()
{
  int    nLen          ;
  char  *pStrPtr       ;
  char   pStr[530]     ;
}

Gleiches gilt natürlich für solche Kommentare:

/*************************************************
 * Kommentar v1.0                                *
 * Funktionsbeschreibung                         *
 *************************************************/

[wxSkip]

@volkard: Meinst du GetStringNumberType()? OK, aber ich habe auch schon Funktionen mit über 1000 Zeilen gesehen...

EDIT: Oh, ich sehe gerade, ich sollte das ostr.close() und das istr.close() in den file-Funktionen entfernen, sonst kommt noch so einer und meint "FILESTREAMS KÖNNEN RAII!!!!!!"

EDIT2: Meinst du vielleicht, ich sollte bei der Funktion einen istringstream nehmen, ein double einlesen und mit dem Ergebnis und fail() das Richtige herausfinden? Das wäre zumindest ähnlich.

[volkard]

wxSkip schrieb:

@volkard: Meinst du GetStringNumberType()?

Nein, nur GetLeadingOnesCount().

[wxSkip]

volkard schrieb:

wxSkip schrieb:

@volkard: Meinst du GetStringNumberType()?

Nein, nur GetLeadingOnesCount().

Na ja, ich muss dir dazu sagen, dass BIN() ein Makro ist, das ein Template aufruft, welches die Zahl statisch konvertiert. Diese Funktion wird relativ oft bei UFT-8-Decoding aufgerufen, von daher wollte ich sie entsprechend optimieren.

///bin-to-hex static conversion

template<uint64_t param> struct hex_helper
{
    enum
    {
        val = (uint16_t(bool(param & 0x1000000000000000ull)) << 15)
        | (uint16_t(bool(param & 0x100000000000000ull)) << 14)
        | (uint16_t(bool(param & 0x10000000000000ull)) << 13)
        | (uint16_t(bool(param & 0x1000000000000ull)) << 12)
        | (uint16_t(bool(param & 0x100000000000ull)) << 11)
        | (uint16_t(bool(param & 0x10000000000ull)) << 10)
        | (uint16_t(bool(param & 0x1000000000ull)) << 9)
        | (uint16_t(bool(param & 0x100000000ull)) << 8)
        | (uint16_t(bool(param & 0x10000000ull)) << 7)
        | (uint16_t(bool(param & 0x1000000ull)) << 6)
        | (uint16_t(bool(param & 0x100000ull)) << 5)
        | (uint16_t(bool(param & 0x10000ull)) << 4)
        | (uint16_t(bool(param & 0x1000ull)) << 3)
        | (uint16_t(bool(param & 0x100ull)) << 2)
        | (uint16_t(bool(param & 0x10ull)) << 1)
        | (uint16_t(bool(param & 0x1ull)))
    };
};

#define BIN(value) (utils::hex_helper<(0x##value##ull)>::val)

Sieht allerdings auch nicht besser aus...

[!rr!rr_.]

ich verstehe

void bla(foo)
{
    last;
}

trotzdem nicht ganz - daß die auf void bla(foo) folgende Zeile die Anfangszeile des Funktionsinhalts ist, ist doch nicht so überraschend, und wird außerdem durch die Einrückung (ich kenne niemanden, der nicht einrückt) sowieso schon optisch hervorgehoben. Warum noch eine zweite Hervorhebung durch Leerzeile.

[wxSkip]

!rr!rr_. schrieb:

ich verstehe

void bla(foo)
{
    last;
}

trotzdem nicht ganz - daß die auf void bla(foo) folgende Zeile die Anfangszeile des Funktionsinhalts ist, ist doch nicht so überraschend, und wird außerdem durch die Einrückung (ich kenne niemanden, der nicht einrückt) sowieso schon optisch hervorgehoben. Warum noch eine zweite Hervorhebung durch Leerzeile.

1. Gewohnheitssache.
2. Ich finde es optisch schöner, weil ich es gewöhnt bin und weil die Klammern auf gleicher Höhe sind.
3. Es ist einfacher mit der Code::Blocks Sonderzeichenergänzung zu kombinieren.

[volkard]

wxSkip schrieb:

...

Hast Du gemessen, daß asm bsr oder __builtin_clz langsamer sind?

[wxSkip]

volkard schrieb:

wxSkip schrieb:

...

Hast Du gemessen, daß asm bsr oder __builtin_clz langsamer sind?

Was ist das? Sieht nicht nach Standard-C++ aus.

EDIT: __builtin_clz ist genau das Gegenteil von dem, was ich brauche (gibt führende Nullen zurück), asm bsr lässt sich leider nicht so gut finden. Auf GCC-Spezifische Sachen würde ich höchstens mit einem #ifdef zugreifen, inline-Assembler fände ich nicht so gut.

[volkard]

wxSkip schrieb:

volkard schrieb:

wxSkip schrieb:

...

Hast Du gemessen, daß asm bsr oder __builtin_clz langsamer sind?

Was ist das? Sieht nicht nach Standard-C++ aus.

Und? Für außergewöhliche Architekturen kann man ja suboptimalen Standard-Code machen.

size_t utils::binary::GetLeadingZerosCount(char_t in)
{
    return GetLeadingOnesCount(~in);
}

[wxSkip]

Ah, gut... (*hirn an*)
char_t ist bloß leider meistens 32 Bits groß und der char ja bekanntlich 8 Bit... während unsigned int eher undefiniert groß ist :p ... Mal schauen.
Und wenn ich den char von GetLeadingOnesCount in ein unsigned int konvertiere, dann __builtin_clz aufrufe und dann vom Ergebnis (sizeof(unsigned int)-sizeof(char)) * 8 abziehe, habe ich da so meine Zweifel, ob das schneller ist, zumal bei UTF-8 in den meisten Fällen gleich die erste if-Bedingung true liefern wird.

[volkard]

wxSkip schrieb:

Ah, gut... (*hirn an*)
char_t ist bloß leider meistens 32 Bits groß und der char ja bekanntlich 8 Bit... während unsigned int eher undefiniert groß ist :p ... Mal schauen.

Ups, eine Asymmetrie der beiden Funktionen, die mir nicht aufgefallen ist (Gefahr).
(Vielleicht auch hier schauen. http://graphics.stanford.edu/~seander/bithacks.html#IntegerLogDeBruijn )

wxSkip schrieb:

Und wenn ich den char von GetLeadingOnesCount in ein unsigned int konvertiere, dann __builtin_clz aufrufe und dann vom Ergebnis (sizeof(unsigned int)-sizeof(char)) * 8 abziehe, habe ich da so meine Zweifel, ob das schneller ist, zumal bei UTF-8 in den meisten Fällen gleich die erste if-Bedingung true liefern wird.

Das wäre ein schöner Kommentar für die GetLeadingOnesCountByIfCascadeForUTF8AssumingAlmostAllInputReturningZero().

Nicht, daß es wirkt, als wolle ich Deinen Code nur schlechtmachen.
Erstmal für Deinen Stil. Den kann ich gut lesen.