Keskustelu: Ohjelmointikysymykset: C++: Muistiongelma (tai käyttäjävirhe ;)

Moi!

Edellinen aloitus saikin mielenkiintoisen keskustelun aikaiseksi moniulotteisista tauluista :D Nyt haluaisin tietää, mikä on ongelmana seuraavassa koodin pätkässä? Itsellä ei vielä ole muistin hallinta täysin hyppysissä, joten vinkkejä kaivataan.

Funktio char *toBinaryString(unsigned int tai unsigned short) palauttaa, joko 12 tai 17 merkkiä pitkän "binääri" stringin tapauksesta riippuen. Esim. annetaan funktiolle sisään nolla (0) niin se palauttaa, joko 12 tai 17 merkkiä pitkän stringin täynnä nollia jne. (älä kysy miksi, se on vain sen tehtävä :) )

//Includet ja toBinaryString() esittely...

int main(int argc[], char *argv[]){

char **stringTaulu;
stringTaulu = (char**)malloc(sizeof(char*));

for(int i=0;i<118;i++){
stringTaulu[i] = (char*)malloc(sizeof(char)*18); //Jokaisen alkion on tarkoitus säilöä max. 17 merkkiä pitkä stringi.

//Keinotekoinen täyttö
strcpy(stringTaulu[i], toBinaryString(i));
}

for(int j=117;j=0;j--){
free(stringTaulu[j]);
}
free(stringTaulu);

return 0;
}

//toBinaryString() -funktio

Eli muistin varaus ja vapautus toimii omasta mielestäni oikein, ainakin tiettyyn pisteeseen... tarkkaan ottaen kun yritän tallentaa 13:sta binary stringiä niin ohjelma kaatuu. 12:Sta stringiin asti kaikki toimii moitteetta.

Ps. Kaivataan taas hyödyllisiä neuvoja, eikä pelkkiä kommetteja "Mene lukemaan c syntaksia" =)

stringTaulu = (char**)malloc(sizeof(char*));
for(int i=0;i<118;i++)
// ...
for(int j=117;j=0;j--)

Et varaa siihen kuin yhden char*-alkion, mutta kuitenkin yrität käyttää 118 alkiota. Lisäksi tuossa jälkimmäisessä silmukassa ehdossasi on virhe. Sijoitat j = 0, jolloin lopputuloksena silmukkaa ei suoriteta kertaakaan. Jos olet tarkoittanut j != 0, tulisit vapauttaneeksi kaikki paitsi nollannen, tai jos j == 0, et taas vapauttaisi yhtään, koska eihän j ole nolla. Oikein olisi j >= 0, jolloin silmukkaa suoritettaisiin kaikille j:n arvoille nollasta alkuarvoon. Kukaan ei kuitenkaan pakota vapauttamaan tuossa järjestyksessä, voit tehdä samanlaisen silmukan kuin alussakin.

stringTaulu = (char**)malloc(sizeof(char*) * RIVEJA);
for (int i = 0; i < RIVEJA; ++i) // Sekä alussa että lopussa

Helpommalla varauksen puolesta pääset varaamalla kerran ja asettamalla osoittimet osoittamaan varaamaasi paikkaan:

const int RIVEJA = 118;
const int MERKKEJA = 18;
// Joka riviä kohden tarvitaan osoitin ja tilaa merkeille
stringTaulu = (char**)malloc((sizeof(char) * MERKKEJA + sizeof(char*)) * RIVEJA);
// Ensimmäinen teksti alkaa siitä, mihin osoittimet loppuvat
stringTaulu[0] = (char*)(stringTaulu + RIVEJA);
for (int i = 1; i < RIVEJA; ++i) {
    // Loput rivit (alkaen ykkösestä) tulevat aina oikean merkkimäärän jälkeen seuraavasta
    stringTaulu[i] = stringTaulu[i-1] + MERKKEJA;
}

free(stringTaulu); // Vain yksi vapautus, kun vain kerran varattiinkin.

Lopuksi pitää vielä muistuttaa, että C++ ja malloc-kikkailut eivät kuulu yhteen ilman todella hyvää syytä... Mutta tämähän oli nyt toivottavasti C:tä, jos oikein käsitin. :)

Onhan tätä aihetta muutamaan kertaan kaluttu...

Millä tavoin toBinaryString hankkii sen palauttamansa osoitteen? Osoitteleeko se funktion omaan static-taulukkoon vai allokoiko funktio muistia dynaamisesti? Jos allokoi, niin miten?

Onko tarkoitus tehdä homma C:llä vai C++:lla?

Tässä on ko. funktion koodi 17 bitin versiota varten ja 12 bitin versiota varten, muuttuvat vain taulukoiden koot sekä nollien lasku.. "zeros" sekä zeroString..

char *toBinaryString(unsigned short data){
	char string[18];
	char tempString[18];
	char zeroString[18];
	unsigned short zeros;

	_itoa(data, string, 2);
	zeros = 17 - strlen(string);

	if(zeros>0){
		strcpy(zeroString, "00000000000000000");	//17 zeros
		strcpy(tempString, string);
		strncpy(string, zeroString, zeros);
		string[zeros] = '\0';
		strcat(string, tempString);
		string[18] = '\0';
	}

	return string;
}

Ja sitten kommentti, josta varmasti pidät :DD hehe: Tarkotus on saada vaan ohjelma toimimaan sen tarkemmin katsomatta kummalla se on tehty. Luultavammin aika sekanen syntaksi on ;D

Tuo ei nyt vain toimi ihan noin. Funktion sisällä syntyy taulukko string. Funktio palauttaa osoittimen tuohon taulukkoon, mutta samalla kun funktiosta palataan, taulukko lakkaa olemasta. Palautettu osoitin osoittelee siis vähän minne sattuu, jossa voi olla mitä sattuu.

Homma onnistuu, jos string:in määrittelyyn lisätään static-määrä. Tällöin funktio käyttää aina samaa taulukkoa. Peräkkäiset kutsukerrat kirjoittavat edellisten tietojen päälle.

Toinen vaihtoehto on, että funktio varaa string-taulukon dynaamisesti. Tällöin kutsujan pitää sitten muistaa vapauttaakin se taulukko joskus, muuten tulee muistivuotobugi.

Kolmas vaihtoehto on, että määritellään struct, jonka sisällä taulukko on. Funktio määritellään sitten palauttamaan tällaisen struct:in. Etenkin suuremmilla taulukoilla tällainen voi olla tehotonta, koska paluuarvoja saatetaan joutua kopioimaan.

Taulukko voidaan tietysti myös antaa parametrina funktiolle, jolloin homma toimiikin sitten ihan eri tavalla. Muitakin vaihtoehtoja on, mutta ne ovat sitten selkeästi C++:aa ja kylläkin paljon yksinkertaisempia ja turvallisempia.

Täsmälleen saman koodin käyttäminen sekä C:ssä että C++:ssa ei ole hyvä idea. Sama koodi kyllä yleensä kääntyy, mutta C++:ssa asiat nyt vaan tehdään eri filosofian mukaan. Jos C++:ssa käyttää C-koodia ja erityisesti C:n muistinhallintafunktioita, tuollainen koodi pitää eristää tiukasti jonkun luokkarajapinnan tms. taakse. Turma tai ainakin turpaan tulee, jos koodarit joutuu kovin paljon arvuuttelemaan, että pitäisköhän tälle pointterille kutsua free:tä, delete:ä vai kenties jotain muuta. Ja varsinkin kun C++:ssa on näihinkin juttuihin valmiit standardikikkulat.

--Tuo ei nyt vain toimi ihan noin. Funktion sisällä syntyy taulukko string. Funktio palauttaa osoittimen tuohon taulukkoon, mutta samalla kun funktiosta palataan, taulukko lakkaa olemasta. Palautettu osoitin osoittelee siis vähän minne sattuu, jossa voi olla mitä sattuu. --

Jaaahas.. ihan mielenkiintoisia huomautuksia ja yritän muistaa jatkossa, mutta ainakaan tällä hetkellä ei ole yhtään "virheellistä" stringin kopioitumista tapahtunut ja se riittäköön minulle.

RopoMen kirjoitti:

ainakaan tällä hetkellä ei ole yhtään "virheellistä" stringin kopioitumista tapahtunut ja se riittäköön minulle.

Älä hyvä mies koittele onneasi. On onnekas sattuma että tuo hirvitys toimii edes siinä kopioinnissa. En kumminkaan usko että olet edes testannut tuota kunnolla. Seuraavassa pari koukkua siitä, mikä hommassa mättää. Mitä sun mielestä pitäisi tulla ulos tällaisesta pätkästä?

int factorial(int n)
{
  if (n <= 0) return 1;
  return n * factorial(n-1);
}

int main()
{
  printf("\"%s\"\n", toBinaryString(0));
  printf("\"%s\" \"%s\"\n", toBinaryString(1), toBinaryString(2));
  printf("%d \"%s\"\n", factorial(3), toBinaryString(4));
  printf("\"%s\" %d\n", toBinaryString(5), factorial(6));
}

Vastaako tulos odotuksia?

Mutta joo, "kyllä mulla ainakin toimii". Mä en jaksa enää.

Jos muistinhallintaongelman haluaa siirtää funktion käyttäjälle, voi tehdä funktion niin, että käyttäjän pitää antaa char-taulu, johon tuotettu teksti laitetaan, ja suurin sallittu määrä merkkejä:

int toBinaryString(int luku, char *teksti, int max_pituus);