Czy taka funkcja to zło? I czy doprowadza do wycieków pamięci?

Witam Panowie,

Napisałem funkcję, która ma zwracać zaszyfrowany/odszyfrowany napis nie naruszając oryginału:

char * const Zmień(const char* str, const unsigned long len)
{
    static char* result = NULL;
    if(result != NULL) free(p);
    result = (char*)malloc(l);

    for(unsigned long i = 0; i < len; i++) result[i] = str[i] ^ 5;
    return result;
}

Chciałem się Was zapytać czy wykorzystanie statycznego wskaźnika result jako takiego jakby 'pojemnika' to dobry pomysł? Nie chcę się uczyć złych nawyków. Zrobiłem to tak, ponieważ chciałem, aby funkcja była jak najłatwiejsza w użyciu, nie chciałem dawać kolejnych 2 parametrów dla buffora, który zawierałby rezultat i jego długości, ponadto czy na pewno zabezpieczyłem się odpowiednio przed wyciekiem pamięci ?

Wkradł się mały błąd do funkcji, a nie mogę edytować :(
Tutaj poprawka:

char * const Zmień(const char* str, const unsigned long len)
{
    static char* result = NULL;
    if(result != NULL) free(result );
    result = (char*)malloc(len);
 
    for(unsigned long i = 0; i < len; i++) result[i] = str[i] ^ 5;
    return result;
}

dodanie znacznika <code class="cpp"> - fp

Czemu nie uzywasz C++ w C++? Wiekszosc problemow z miejsca zniknie.

string Change(const string& str)
{
    string result(str.size(), 0);
    for(unsigned long i = 0; i < str.size(); i++) result[i] = str[i] ^ 5;
    return result;
}

Lub cos takiego:

string Change(const string& str)
{
    string result;
    transform
    (
        str.begin(),
        str.end(),
        back_inserter<string>(result),
        [](const char v)
        {
            return v ^ 5;
        }
    );
    
    return result;
}

n0name_l napisał(a):

Czemu nie uzywasz C++ w C++? Wiekszosc problemow z miejsca zniknie.

string Change(const string& str)
{
    string result(str.size(), 0);
    for(unsigned long i = 0; i < str.size(); i++) result[i] = str[i] ^ 5;
    return result;
}

Przekazywanie przez referencję, żeby później kopiować nie jest fajne. Niech kompilator sobie sam kopiuje. (http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/) Dodatkowo, pętlę można ładnie zastąpić w C++11.

string Change(string str) {
  for (char &ch : str) {
    ch ^= 5;      
  }
  return str;
}

W drugim kodzie lambdę można zastąpić std::bit_xor (razem z std::bind), ale i tak będzie brzydko wyglądać. ;-)

Jest źle, bo co jeśli chcesz użyć tę funkcję dwa razy po rząd i mieć oba wyniki?
Najlepiej trzymać się pewnych standardów i zrobić to tak:

void copyEndcoded(const char* src, const unsigned long len, char* dest)
{
    for(unsigned long i = 0; i < len; i++) dest[i] = src[i] ^ 5;
}

char *allocEncoded(const char* src, const unsigned long len)
{
    char *result = (char*)malloc(len*sizeof(char));
    copyEndcoded(src, len, result);
    return result;
}

Nazwa funkcji od razu pokazuje mi, że abo wykonuję kopię w zadanym miejscu, albo kopię na stosie, za którą kod wywołujący ma wziąć odpowiedzialność.

Oczywiście chodzi mi o język C, przez pomyłkę dałem w tagu C++.

@MarekR22 też myslalem, żeby zrobić tak jak Twój drugi przyklad, ale mam pewnosc, że zwracana wartość za pomocą tej funkcji nie będzie nigdzie przypisywana tylko używana w tym sposób:

MessageBox(0, Zmien(napis), "info", 0);

Mam 100% pewność, że nigdy nie będę potrzebował przechowywać wynik tej funkcji w jakimś wskaźniku i dalej na nim pracować, a jeśli mimo to miałbym zastosować twój przykład, musiałbym zadbać o zwolnienie pamięci, a więc musiałbym jednak zwrócona wskaznik gdzieś przypisać. Czy wiedząc o tym, mogę tak zostawić i nie martwić się o wyciek pamięci ?

Jesli masz jakies gorne ograniczenie to mozesz zrobic tak:

nts:

char* copyEndcoded(const char* src, const unsigned long len)
{
    static char buffer[1024];
    for(unsigned long i = 0; i < len; i++) buffer[i] = src[i] ^ 5;
    return buffer;
}

lub:

char* copyEndcoded(const char* src, char* buffer const unsigned long len)
{
    for(unsigned long i = 0; i < len; i++) buffer[i] = src[i] ^ 5;
    return buffer;
}

I gdzies wczesniej zadeklarowac.

Wtedy sie nie martwisz o zwalnanie pamieci, bo wszystko jest statyczne/na stosie.

Jesli uzyjesz pierwotnego sposobu, ktory przedstawil @MarekR22, to bedziesz musial korzystac z tego tak:

char* buf = allocEncoded(...);
MessageBox(...);
free(buf);

Gumś napisał(a):

Oczywiście chodzi mi o język C, przez pomyłkę dałem w tagu C++.

@MarekR22 też myslalem, żeby zrobić tak jak Twój drugi przyklad, ale mam pewnosc, że zwracana wartość za pomocą tej funkcji nie będzie nigdzie przypisywana tylko używana w tym sposób:

MessageBox(0, Zmien(napis), "info", 0);

Mam 100% pewność, że nigdy nie będę potrzebował przechowywać wynik tej funkcji w jakimś wskaźniku i dalej na nim pracować, a jeśli mimo to miałbym zastosować twój przykład, musiałbym zadbać o zwolnienie pamięci, a więc musiałbym jednak zwrócona wskaznik gdzieś przypisać. Czy wiedząc o tym, mogę tak zostawić i nie martwić się o wyciek pamięci ?

Jeśli koniecznie chcesz się pakować w takie kłopoty (na99% tak się to skończy), to rób to z głową. Zrób dobra nazwę funkcji, żeby było wiadomo, że wkładasz palce między drzwi, gdy za 4 miesiące będziesz miał jakiś dziwny błąd.
Przykładowo nazwanie tej funkcji tempEndcoded już sygnalizuje, że coś nie tak jest z czasem życia wyniku tej funkcji i jak będziesz miał jakiś dziwny problem, szybko odkryjesz w czym problem, zamiast walczyć z nim kilka dni.
Nadawanie nazw w programowaniu to naprawdę ważna sprawa.